Don't change the meaning of string literals

[nvie]

This line singlehandedly has caused a slew of painful bugs for us in production since we upgraded from 2.0.8 to 2.0.9, seeing various UnicodeDecodeErrors on repositories with branches/tags/etc that have non-ASCII characters in them. For most repos that only use the ASCII alphabet, implicit encoding and decoding is now happening, and most of the time we get lucky because they accidentally just work.

After reverting this one liner, our production issues all went away.

@ankostis What's the reason this line was included in f11fdf1? It's the only module in the GitPython code base now that uses unicode literals, which makes reasoning about the code related to strings in this module rather confusing. Could we perhaps just change the strings that should be unicode for your patch and mark those with u'...' string literals explicitly rather than changing the meaning of all strings at once?

[ankostis]

Dear @nvie I deeply apologize for the problems this change has caused you.
You are right, there is no reasons for this change or any explanation in the commit-msg,
so I deduce it could have been a mistake on my part, while trying various fixes for Windows & PY3,
and forgetting to revert them before commit

What is surprising is that no TC fails when reverting this line.
So it indeed there was no real reason for this change.

What is unsettling is that we have no TCs to detect the problems you had.
Can you help into building such a test-case?

[nvie]

Dear @nvie I deeply apologize for the problems this change has caused you.

No need to! We've all been there ;)

You are right, there is no reasons for this change or any explanation in the commit-msg,
so I deduce it could have been a mistake on my part, while trying various fixes for Windows & PY3,
and forgetting to revert them before commit.

I thought that might have been the case since it's so isolated, just wanted to double-check this.

What is surprising is that no TC fails when reverting this line.
What is unsettling is that we have no TCs to detect the problems you had.
Can you help into building such a test-case?

I thought of this too, but the bugs are so subtle and hard to replicate that I didn't have a good place to start adding them. The original bugs we have been seeing popped up in a whole different section of the code, so it was quite a journey to end up here, finding this was the root cause.

FWIW, I've included the stack trace that triggered out bug hunt here.

Example git-fetch output that causes this bug to reveal itself:

= [up to date]          feature/mybranch                -> feature/mybranch
= [up to date]          feature/foo-‘bar’               -> feature/foo-‘bar’
= [up to date]          feature/anotherbranch           -> feature/anotherbranch

Note the "smart quotes" in the 2nd line. That's an example of a repo that will fail.

Stack trace does not reveal any key source code position. I had to trace the data at each stack level and see where data was "wrong", then try to trace back how that data came to be that way.

Traceback (most recent call last): 
  File "/app/src/foo.py", line 136, in _update 
    custom_fetch(git_repo, progress, url, '+refs/*:refs/*') 
  File "/app/src/lib/bar.py", line 48, in custom_fetch 
    return remote._get_fetch_info_from_stderr(proc, progress) 
  File "/app/.venv/src/gitpython/git/remote.py", line 638, in _get_fetch_info_from_stderr 
    proc.wait(stderr=stderr_text) 
  File "/app/.venv/src/gitpython/git/cmd.py", line 290, in wait 
    raise GitCommandError(self.args, status, errstr) 
GitCommandError: Cmd('git') failed due to: exit code(-13) 
  cmdline: git fetch --progress -v https://****:****@github.com/someuser/somerepo.git +refs/*:refs/* 
  stderr: 'fatal: repository 'https://****:****@github.com/someuser/somerepo.git/' not found' 

As you can see, this stack trace is of no help finding this bug.

[nvie]

Thanks for the quick response, though. I went ahead and merged this one myself.

[ankostis]

unicode_literals were forgotten from a Q'nD experiment while fighting with Win+PY3 errors.

[nvie]

@Byron Could you perhaps roll a release of GitPython to PyPI that includes this fix? Would be appreciated!

[Byron]

@nvie Will have to work through more emails and PRs, but will cut a new release when done. That should happen today as well.

[nvie]

Thanks, no rush!

[ankostis]

So @nvie, the unicode problem happens only with fetch cmd?
Or have you encountered problems with unicode-branches in other commands?

Just linking #550 which might also relate to strangely-named branches - so that we can craft a TC for all these problems.

[nvie]

@ankostis Yes and no. We thought it was fetch-related for a long time at first, because that's the only place it popped up for us. I think more Git commands could be affected however. The specific bug happened on this line in the end. Because of the unicode_literals, this was trying to see if a bytes string (input of the function) started with the now-unicode string literals given on that line ('error' and 'fatal'). This caused the implicit decoding step and the failure.

This particular one happened in the RemoteProgress class, so anything using that would surely be affected, but the same bug is more wide-spread than just this class, since in a way this class has nothing to do with it. Basically any place that does a .startswith('whatever') (or whatever other method on strings that cause implicit string decoding) is a potential hiding place for this bug.

[nvie]

Here's one idea to systematically find all places affected:

1. Find all places where a x.startswith(y) is called.
2. Put an explicit assert type(x) == type(y), "A bug is lurking here" above the line.
3. Try touching all these code paths.

Any place where this fails is a similar bug.