Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork939
Fix order of operators before executing the git command#431
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Since Python 3.3, the hash value of an object is seeded randomly, making itchange between each call. As a consequence, the `dict` type relying on the hashvalue for the order of the items upon iterating on it, and the parameterspassed to `git` being passed as `kwargs` to the `execute()` method, the orderof parameters will change randomly between calls.For example, when you call `git.remote.pull()` in a code, two consecutives runwill generate:1. git pull --progress -v origin master2. git pull -v --progress origin masterWithin 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 theparameters in the same order.
Also, for the note, Ido 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 -U The 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change causesGitPython to be no longer compatible with Python 2.6, even ifhttps://pypi.python.org/pypi/ordereddict is installed...
Is that the intention, or should this be replaced with:
from git.odict import OrderedDictas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Of course: You will find these announcements in thechangelog.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Byron I agree it's worth fixing, especially sincegit.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 leastknowing that Py2.6 compatibility is broken.
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 very much like the idea - a commit is on the way to see what happens.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
Uh oh!
There was an error while loading.Please reload this page.
Hello,
For a tool I'm writinggit-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, andhere 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 thetravis 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.