Fix order of operators before executing the git command #431
Conversation
Since Python 3.3, the hash value of an object is seeded randomly, making it change between each call. As a consequence, the `dict` type relying on the hash value for the order of the items upon iterating on it, and the parameters passed to `git` being passed as `kwargs` to the `execute()` method, the order of parameters will change randomly between calls. For example, when you call `git.remote.pull()` in a code, two consecutives run will generate: 1. git pull --progress -v origin master 2. git pull -v --progress origin master Within the `transform_kwargs()` method, I'm promoting `kwargs` into an `collections.OrderedDict` being built with `kwargs` sorted on the keys. Then it will ensure that each subsequent calls will execute the parameters in the same order.
|
Also, for the note, I do know that I could setup But I'd rather avoid that, as it would introduce a difference with real world usage of the tool (and associated libraries), and if a regression based on order of a |
| @@ -783,6 +785,7 @@ def transform_kwarg(self, name, value, split_single_char_options): | |||
| def transform_kwargs(self, split_single_char_options=True, **kwargs): | |||
| """Transforms Python style kwargs into git command line options.""" | |||
| args = list() | |||
| kwargs = OrderedDict(sorted(kwargs.items(), key=lambda x: x[0])) | |||
There was a problem hiding this comment.
nitpick: you could use operator.itemgetter() for more readability
|
Thank you for all the time you put in to find this one ! It looks like a sensible thing to do even though doing this might be a breaking change for some, where the kwargs magically turned out to be in the correct order. Generally, doing this deterministically is certainly preferred, even though there is a certain risk involved. |
|
You're welcome, and thank you for the lib! Though, if you want an advice, you should drop the |
|
Thanks for the advice ! I certainly would remove it if I just knew where to find it. ag PYTHONHASHSEED -UThe line above didn't yield any result. |
|
ok, it might be my flu medication, I would have sworn to have seen it setup somewhere in the repo before I went sick :-s so, nevermind ☺ |
| @@ -13,6 +13,8 @@ | |||
| import errno | |||
| import mmap | |||
|
|
|||
| from collections import OrderedDict | |||
There was a problem hiding this comment.
This change causes GitPython to be no longer compatible with Python 2.6, even if https://pypi.python.org/pypi/ordereddict is installed...
Is that the intention, or should this be replaced with:
from git.odict import OrderedDict
as is done here: https://github.com/gitpython-developers/GitPython/blob/master/git/config.py#L20 ?
I'm happy to issue a pull request for this, as I would really like GitPython to remain compatible with Python 2.6 for now...
Should .travis.yml be updated as well to test against Python 2.6 + ordereddict?
There was a problem hiding this comment.
Hi @boegel,
Indeed, with release 2.0 we officially dropped support for py2.6, assuming that this move is fine as py2.6 is not maintained anymore.
Nonetheless, I see no harm in using the alternate odict implementation, and would very much welcome a PR.
Thank you!
There was a problem hiding this comment.
Python 2.6 is still the default Python in several operating systems, for example anything Red Hat 6.x based (CentOS 6, RHEL 6, Scientific Linux 6, etc.).
But, I understand the move, the only way is forward, I guess we should just stick to the latest GitPython 1.x then on these systems.
Would you mind sharing a pointer to the official announcement that mentions dropping Py2.6 support?
There was a problem hiding this comment.
Of course: You will find these announcements in the changelog.
The cause of the move was some minor detail (which I forgot), so I believe after fixing this issue, 2.0 has a good change to run in py2.6 anyway. It's worth a try, considering how trivial the odict fix seems to be.
There was a problem hiding this comment.
@Byron I agree it's worth fixing, especially since git.odict is still there.
Should I also look into adding (back) testing for Py2.6 in .travis.yml, indicating that the tests for Py2.6 are allowed to fail? I feel that may help in at least knowing that Py2.6 compatibility is broken.
There was a problem hiding this comment.
I very much like the idea - a commit is on the way to see what happens.
There was a problem hiding this comment.
Here is the result: https://travis-ci.org/gitpython-developers/GitPython/jobs/133041770 .
Seems to be just one part of the compat code, which may be fixable in some way in case the blame functionality is important to someone.
There was a problem hiding this comment.
for future reference: this issue got fixed in #431, but other incompatibilities with Py2.6 remain, e.g. https://travis-ci.org/gitpython-developers/GitPython/jobs/133041770
I won't be spending time on those (for now), I'll just stick with the latest GitPython 1.x on Py2.6 (cfr. easybuilders/easybuild-framework#1781)
Hello,
For a tool I'm writing git-repo, I was working with a mockup of subprocess to run regression tests for commands issued through GitPython. I've written a process similar to the VCR or Betamax libraries to define expected command calls passed through Popen. Here is where I setup the mockup, and here is a place where I use it.
It was working great, up until recently when I've introduced the
--progresssupport. Then I've been hitting an issue where the same code would fail inconsistently between calls. Cf the travis reports.With a fair bit of debugging, I found out the issue is that because GitPython relies on the
kwargsconstruct to pass around optional arguments, the order of argument will depend on the order of the items within the dict. As when I introduced progress support,opt_argcontains'-v'and'--progress'now, the order depending on bothhash('-v')andhash('--progress'). So this request for merging is intended to fix that issue by simply ordering optional arguments by their name before passing the command to subprocess.