Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 19, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix order of operators before executing the git command
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.
  • Loading branch information
guyzmo committed May 12, 2016
commit 89ade7bfff534ae799d7dd693b206931d5ed3d4f
3 changes: 3 additions & 0 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import errno
import mmap

from collections import OrderedDict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

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!

Copy link
Contributor

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?

Copy link
Member

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 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.

Copy link
Contributor

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 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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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)


from contextlib import contextmanager
import signal
from subprocess import (
Expand Down Expand Up @@ -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]))

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

for k, v in kwargs.items():
if isinstance(v, (list, tuple)):
for value in v:
Expand Down