-
-
Notifications
You must be signed in to change notification settings - Fork 935
Pr cmd raise with stderr on error #452
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
Pr cmd raise with stderr on error #452
Conversation
Hey Barry, Thanks a bunch for your contribution ! Could you also have a look at travis please ? It seems that a few tests are failing. They probably need to be adjusted or something did break indeed. You will find everything you need to run the tests in the Thanks |
for line in proc.stderr: | ||
line = force_text(line) | ||
for pline in progress_handler(line): | ||
if line.startswith('fatal:') or line.startswith('error:'): | ||
raise GitCommandError(("Error when fetching: %s" % line,), 2) | ||
error_message = "Error when fetching: %s" % (line,) | ||
break |
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.
Are you sure it' s enough to break after the first line indicating a problem ? My intuition is that the lines that follow on stderr contain context, that you probably want to catch as well.
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.
It works because of the 2nd break at line 591.
Also I see this work in testing.
Found flake8 in pip and used to to fix the complaints. Took a moment to figure out where nosetests came from. I see errors from running the tests I'll work on there a bit later. may be tomorrow before I can push a update. |
remove stderr for a wait() that is not the GitPython wrapper.
Good to hear you can now test locally ! With all that in place, you should reach a point where all tests run successfully on your own machine, in case everything works. |
I have instrumented to try and find out what is happening in one of the test failures. What I see is this command that fails with 128. git clone --progress --separate-git-dir=/home/barry/tmpdir/test_renamescm7sp1x/parent/.git/modules/mymodules/myname -v /home/barry/wc/git/GitPython/git/ext/gitdb/gitdb/ext/smmap /home/barry/tmpdir/test_renamescm7sp1x/parent/mymodules/myname Notice the /home/barry/wc/git/GitPython/git/ext/gitdb/gitdb/ext/smmap path. |
Sorry I missed your reply about testing. Work on master?! But isn't that a git sin? Are you saying that I need to run all the commands in the install: section? |
Convert to the expected bytes.
This failure is becuase the GIT_SSH is not making it in to Popen call.
Why expect FOO to be missing an python 2 and darwin?
|
Can you try this and see if the tests pass for you? I fixed the test that looked fro FOO, test case error. Given I'm not on master I see that test fail as well. I am seeing some odd errors from test_repo, but suspect that I have triggered a bug somewhere else. |
Please see if my merge is still working. If not, please add a test that fails unless the fix you provide is applied. |
I have seen the stderr for both pull() and push() in GitCommandError with this patch.