Skip to content

remote: compatibility with git version > 2.10 #623

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 29, 2017

Conversation

wusisu
Copy link
Contributor

@wusisu wusisu commented May 3, 2017

I found that a little change which affects the behavior of git.remote.FetchInfo as git version greater than or equal with 2.10.x. It case git.test.test_remote.TestRemote.test_base fails.

I figure out that a commit of git changes git fetch --tags's output.
And it seems that that commit first appeared in git 2.10.0

➜  git git:(master) git log --oneline v2.9.3..v2.10.0 | grep 2cb0
2cb040b fetch: change flag code for displaying tag update and deleted ref

I have almost no contribution on github. Beg pardon if I am not doing it well.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution, and please know that I am sorry for getting back to this project with such a delay.

There is just one question left, and once clarified this PR should be ready to merge.

@@ -629,7 +634,7 @@ def _get_fetch_info_from_stderr(self, proc, progress):
fetch_info_lines = list()
# Basically we want all fetch info lines which appear to be in regular form, and thus have a
# command character. Everything else we ignore,
cmds = set(PushInfo._flag_map.keys()) & set(FetchInfo._flag_map.keys())
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this change was required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I make a mistake because I may not understand the project deeply.

I make this change for 3 reasons:

  1. In the project before this commit, there is no difference between set(PushInfo._flag_map.keys()) & set(FetchInfo._flag_map.keys()) and set(FetchInfo._flag_map.keys()).
    PushInfo._flag_map.keys() == ['X', '-', '*', '+', ' ', '=', '!'] and FetchInfo._flag_map.keys() == ['!', '+', '-', '*', '=', ' '], and these two _flag_map are considered unchangable.

  2. It seems that there is no need to make an intersection between Push and Fetch.
    The only two usages of this function is fetch and pull. As the function (named _get_fetch_info_from_stderr) has the word fetch in its name, its duty is to parse data from fetch messages, and thus should only use FetchInfo._flags_map.
    So, I think use set(FetchInfo._flag_map.keys()) is just the right way.

  3. Push command has no 't'.
    Accoding to the documents of git-fetch and git-push, fetch has more command than push now, make an intersection will make this function loss some types of records.

That's way I think I could and have to make this change.

@Byron Byron merged commit c121f60 into gitpython-developers:master May 29, 2017
@Byron
Copy link
Member

Byron commented May 29, 2017

Thanks a lot for the clarification @wusisu , that makes sense.
Interestingly I took this intersection as union, and was afraid that something gets lost without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants