-
-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix git init issue #37
Conversation
@@ -238,6 +238,10 @@ var testCasesExtractBranch = []struct { | |||
[]byte("## Initial commit on master"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure when this message gets printed. Is this for an older version of git
? Maybe we should put a comment to describe how to replicate it in a real system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if perhaps this is in fact the same message and it just changed text in more recent versions of git. Perhaps a search through the git codebase could tell us that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, looks like it was changed in git/git@4ddb135
Awesome! Thanks for taking this on. |
Does git ever localize this output? (separate issue maybe)
…On Wed, Mar 21, 2018, 12:52 Matthew Rothenberg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In commands/status/process_test.go
<#37 (comment)>:
> @@ -238,6 +238,10 @@ var testCasesExtractBranch = []struct {
[]byte("## Initial commit on master"),
Ah yeah, looks like it was changed in ***@***.***
<git/git@4ddb135>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABM8TmT2mxbji9CZWUdsL7i7ovYRcp-Sks5tgoUdgaJpZM4Sy779>
.
|
Is there anything left to be done before we can merge this? |
Sorry for the delay, I was out of the country. The one thing I was thinking is that this is a case we should probably have an integration test for as well, so we automatically catch it if the language changes again or similar. |
Good point. But actually, there is already a test case for it:
So, currently the tests are not even passing on master with the newer git version. I created a PR below just to demonstrate this on CI: #36 |
Oh great, I somehow forgot that integration test already existed. This should be ready to be merged then. Thanks so much for your contribution @zommerfelds ! |
Attempt to fix #36