Command `git merge-base --is-ancestor` not working as expected

bwrsandman commented 9 years ago

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.

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'

Byron commented 9 years ago

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:

# this would only work if revA is a full 40 character hexadecimal SHA1
revA in repo.merge_base(revA, revB)

A quick check of the underlying cgit code showed that the work done there is comparable: The --is-ancestor flag ends up calling in_merge_bases_many(...), whereas the standard-path will end up calling get_merge_bases_many_dirty(...) - both of which seem to do the majority of the work in paint_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:

def is_ancestor(repo, a, b):
    try:
        repo.git.merge_base(a, b, is_ancestor=True)
        return True
    except GitCommandError as err:
        if err.status == 1:
            return False
        raise

Please let me know what you think.

bwrsandman commented 9 years ago

Thanks for the compliment :smile:

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 make a clear that it is the ancestor and perhaps a docstring.

Byron commented 9 years ago

The in workaround is interesting, I didn't think to try is as two separate branches could have a base that is different.

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 commented 9 years ago

I'll make the PR shortly

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.

Missing password or interface change? I only uploaded one package recently, but twine was fairly usable.

Byron commented 9 years ago

I'll make the PR shortly

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 commented 8 years ago

A new release was just made to pypi 😁 (see #298) !

joejuzl commented 6 years ago

Should this issue be closed as the feature is released and working ?

Byron commented 6 years ago

Let's do that!

idbrii commented 1 month ago

For any future readers looking at this PR to see how to use --is-ancestor, the "Desired Behaviour" with is_ancestor=True is not the implemented behaviour. Instead it's a new function:

main is ancestor to revA

>>> repo.is_ancestor("main", revA)
True

main is not ancestor to revB

>>> repo.is_ancestor("main", revB)
False

revC doesn't exist

>>> repo.is_ancestor("main", revC)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git merge-base --is-ancestor main revC
  stderr: 'fatal: Not a valid object name revC'

gitpython-developers / GitPython