Gitpython: Progress: issue with bytes/str handling (py3)

[guyzmo]

I still am unable to produce a proper minimal reproduceable example — I've been busy with feature implementation to dig this further, but depending on some context to be determined, in the method_parse_progress_line() of RemoteProgress sometimes the line argument is a bytes string, other times it's an str string.

I've hit the issue with my implementation in guyzmo/git-repo and to fix it I've made the following fix at line 387 of git.util:

self._cur_line = line = line.decode('utf-8') if isinstance(line, bytes) else line

This issue has been noticed with GitPython 2.1.0, I'm upgrading it locally, to see if it hasn't been fixed with 2.1.1, so please pardon me if it's been fixed ☺

[guyzmo]

here's a stack trace where it happens, with gitpython-2.1.1 (I updated since last report):

> git remote add all https://bitbucket.org/atlassian/python-bitbucket
> Popen(['git', 'remote', 'add', 'all', 'https://bitbucket.org/atlassian/python-bitbucket'], cwd=/tmp/tmp_f32zp1_, universal_
newlines=False, shell=None)
> git remote add bitbucket https://bitbucket.org/atlassian/python-bitbucket
> Popen(['git', 'remote', 'add', 'bitbucket', 'https://bitbucket.org/atlassian/python-bitbucket'], cwd=/tmp/tmp_f32zp1_, univ
ersal_newlines=False, shell=None)
> git version
> Popen(['git', 'version'], cwd=/tmp/tmp_f32zp1_, universal_newlines=False, shell=None)
> git pull --progress -v bitbucket master
> Popen(['git', 'pull', '--progress', '-v', 'bitbucket', 'master'], cwd=/tmp/tmp_f32zp1_, universal_newlines=True, shell=None
)
> Pumping 'stderr' of cmd(['git', 'pull', '--progress', '-v', 'bitbucket', 'master']) failed due to: TypeError("a bytes-like object is required, not 'str'",)
Exception in thread Thread-2:
Traceback (most recent call last):
  File "…/git/cmd.py", line 87, in pump_stream
    handler(line)
  File "…/git/util.py", line 483, in handler
    return self._parse_progress_line(line.rstrip())
  File "…/git/util.py", line 388, in _parse_progres
s_line
    if len(self.error_lines) > 0 or self._cur_line.startswith(('error:', 'fatal:')):
TypeError: a bytes-like object is required, not 'str'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.5/threading.py", line 914, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.5/threading.py", line 862, in run
    self._target(*self._args, **self._kwargs)
  File "…/git/cmd.py", line 90, in pump_stream
    raise CommandError(['<%s-pump>' % name] + cmdline, ex)
git.exc.CommandError: Cmd('<stderr-pump>') failed due to: TypeError('a bytes-like object is required, not 'str'')
  cmdline: <stderr-pump> git pull --progress -v bitbucket master

I think I'm narrowing the issue to the fact that input is supposed to be bytes (former str()), but all string litterals are unicode over there.

But because the handle_process_output() function is called from _get_fetch_info_from_stderr() with decode_stream=False, the pump function is not decoding the bytes to str, and later in the progress_handler it fails.

I switched all decode_streams to True in git.remote and it's not crashing anymore.

In either cases, with that solution it's yelling:

cmd.py                     585 DEBUG    Popen(['git', 'pull', '--progress', '-v', 'bitbucket', 'master'], cwd=/tmp/tmp4z86lqw9, universal_newlines=True, shell=None)
remote.py                  660 DEBUG    Fetch head lines do not match lines provided via progress information
length of progress lines 2 should be equal to lines in FETCH_HEAD file 1
Will ignore extra progress lines or fetch head lines.

but I believe it's unrelated to the issue at hand.

So we've got two solutions, etiher make the progress handler bytes and str agnostic (as I first suggested), or change the remote.fetch calls to handle_process_output().

[guyzmo]

@Byron any preference or idea on it so I can cook a PR?

[Byron]

@guyzmo Thanks for the elaborate description of the problem and the effort you put in already! As for a PR, I believe to remember that decode_streams being False was required to make that (or other) things work, so I would be careful going down that path. Therefore it seems making the parser agnostic of the input type is a more localised fix that seems preferable.
It would be interesting to understand while the type is changing in the first place too, maybe you discover that while looking into the issue.

[guyzmo]

uuuurgh… I tried to reproduce the issue today, by removing my patch… and now it's working fine…

What. The. Bloody. Hell. O_O

I guess I'm closing the issue for now, and next time it happens I'll try to make a minimal replicable snippet of it. I mean it happened twice, it's likely to hit me a third time, because as we say in french: jamais deux sans trois.

[guyzmo]

damn… it just hit me again. Same circumstances, same situation.

self._cur_line = line = line.decode('utf-8') if isinstance(line, bytes) else line

fixed it.

Though right now I'm focused on doing a new release of git-repo, and I want to stay focused on it. I'll see if I got time to make a minimal snippet next week.

[guyzmo]

cf this error output.

The root of the problem is because I'm mocking up the git calling subprocess calls:

• here's the mockup class
• here's where it's used

[Byron]

@guyzmo Would you like to contribute the fix?

[guyzmo]

I'd be happy to, are you happy with the fix I'm suggesting above?

[Byron]

It looks good to me, all things considered :). I mean it's probably a lost cause to try to get bytes/string handling right in this project, so applying fixes where needed seems to be a suitable way to make life for users a little bit better.

[guyzmo]

👌 I'm adding that in my todo list for the next few days ☺

[yarikoptic]

I wonder if this issue might have been solved by now?

[Byron]

@yarikoptic I hope with the linked commit, this issue will be resolved. It's a certainly a quick stab at it.

[Byron]

Closed due to inactivity - please feel free to comment if there is an interest to pick it up.