Description
Description
Using:
- git version 2.4.6
- gitdb==0.6.4
- GitPython==1.0.1
The use case for git merge-base --is-ancestor
from git manpage:
Check if the first is an ancestor of the second , and exit with status 0 if true, or with status 1 if not. Errors are signaled by a non-zero status that is not 1.
Related to #169 (comment)
The problem is that the output of git merge-base --is-ancestor
comes from the return code and unlike without the --is-ancestor
flag, this command doesn't return commit SHAs but is a simple yes-no check. The current implementation doesn't take into account the return code and the command cannot be usefully run.
Current Behaviour
revA is ancestor to revB
$ git merge-base --is-ancestor $revA $revB; echo $?
0
>>> repo.merge_base(revA, revB, is_ancestor=True)
[ ]
revA is not ancestor to revB
$ git merge-base --is-ancestor $revA $revB; echo $?
1
>>> repo.merge_base(revA, revB, is_ancestor=True)
[ ]
revA is not ancestor to revB and revB doesn't exist
$ git merge-base --is-ancestor $revA $revB; echo $?
fatal: Not a valid commit name $revB
128
>>> repo.merge_base(revA, revB, is_ancestor=True)
GitCommandError: 'git merge-base --is-ancestor $revA $revB' returned with exit code 128
stderr: 'fatal: Not a valid commit name $revB'
Desired Behaviour
When the kwarg
is_ancestor
is used, don't return a list, but return a bool that is true if the return code is 0. Return False if return code is 1. Raise Error as before for all other cases
$ git merge-base --is-ancestor $revA $revB; echo $?
0
>>> repo.merge_base(revA, revB, is_ancestor=True)
True
revA is not ancestor to revB
$ git merge-base --is-ancestor $revA $revB; echo $?
1
>>> repo.merge_base(revA, revB, is_ancestor=True)
False
revA is not ancestor to revB and revB doesn't exist
$ git merge-base --is-ancestor $revA $revB; echo $?
fatal: Not a valid commit name $revB
128
>>> repo.merge_base(revA, revB, is_ancestor=True)
GitCommandError: 'git merge-base --is-ancestor $revA $revB' returned with exit code 128
stderr: 'fatal: Not a valid commit name $revB'
Activity
Byron commentedon Jul 20, 2015
Thanks for taking the time to producing something that is definitely in the top-10 of the best issues this project has ever seen !
Even though I am acknowledging the issue, I wonder whether it is worth fixing. Wouldn't the required information be conveyed if one would do the following:
A quick check of the underlying cgit code showed that the work done there is comparable: The
--is-ancestor
flag ends up callingin_merge_bases_many(...)
, whereas the standard-path will end up callingget_merge_bases_many_dirty(...)
- both of which seem to do the majority of the work inpaint_down_to_common
. The standard-path does more processing, admittedly, still I'd be inclined to believe that both invocations spend more time doing IO than anything else.The reason why I am hesitant to implement the proposed solution is that it would break the existing API in a stable release. Additionally I don't believe altering the return type of any routine based on its arguments is a good thing.
A proposed workaround with exact semantics is to make the git call directly, possibly through a utility function:
Please let me know what you think.
bwrsandman commentedon Jul 20, 2015
Thanks for the compliment 😄
The
in
workaround is interesting, I didn't think to try is as two separate branches could have a base that is different.The
is_ancestor
solution is what I would do. Adding it to the library could make it complete without breaking the api. It's pythonic, stays true to the git semantic and is straight forward. The only things I would change are the parameter names to makea
clear that it is the ancestor and perhaps a docstring.Byron commentedon Jul 21, 2015
It could totally be I am wrong about this, but if I imagine a forked branching model (both sides have seen new commits), then the produced merge-base can't be including the head commits of any side. Probably there are other cases that aren't covered though.
The given code was just an example, and I'd imagine it to be useful in a client codebase.
As I would not make any adjustments in this matter, I suggest to make a PR that contains a fix that works for you. My behaviour is also fuelled by my inability to login to pip and make new releases - it's very discouraging for me to write any python code these days.
bwrsandman commentedon Jul 21, 2015
I'll make the PR shortly
Missing password or interface change? I only uploaded one package recently, but twine was fairly usable.
Byron commentedon Jul 21, 2015
Thank you, looking forward to it.
pypi
used openID2, which is now unsupported by google. However, at that site nobody seems to have noticed, which means those without a native account just can't login anymore using the website. In the old days, I believe one would have to put ones password in clear-text in a file to use command-line tools for uploads, which seemed inacceptable to me, and something like an API-key I never had. Thus I have no way of accessing my account anymore.When I realized this, I also realized that Pypi is stuck in a world that crumbles of age.
Byron commentedon Feb 11, 2016
A new release was just made to pypi 😁 (see #298) !
joejuzl commentedon Sep 13, 2018
Should this issue be closed as the feature is released and working ?
Byron commentedon Oct 13, 2018
Let's do that!
idbrii commentedon Sep 11, 2024
For any future readers looking at this PR to see how to use
--is-ancestor
, the "Desired Behaviour" withis_ancestor=True
is not the implemented behaviour. Instead it's a new function:main is ancestor to revA
main is not ancestor to revB
revC doesn't exist