Skip to content

Command git merge-base --is-ancestor not working as expected #321

Closed
Closed
Command git merge-base --is-ancestor not working as expected#321
Labels
acknowledged

Description

@bwrsandman
bwrsandman
opened on Jul 20, 2015

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

  1. Byron
    added
    acknowledged
    on Jul 20, 2015

  2. Byron commented on Jul 20, 2015

    @Byron
    Byron
    on Jul 20, 2015
    Member

    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.

  3. Byron
    added
    waiting for feedback
    on Jul 20, 2015

  4. bwrsandman commented on Jul 20, 2015

    @bwrsandman
    bwrsandman
    on Jul 20, 2015
    Author

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

  5. Byron commented on Jul 21, 2015

    @Byron
    Byron
    on Jul 21, 2015
    Member

    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.

  6. Byron
    removed
    waiting for feedback
    on Jul 21, 2015

  7. bwrsandman commented on Jul 21, 2015

    @bwrsandman
    bwrsandman
    on Jul 21, 2015
    Author

    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.

  8. Byron commented on Jul 21, 2015

    @Byron
    Byron
    on Jul 21, 2015
    Member

    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.

  9. Byron commented on Feb 11, 2016

    @Byron
    Byron
    on Feb 11, 2016
    Member

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

  10. joejuzl commented on Sep 13, 2018

    @joejuzl
    joejuzl
    on Sep 13, 2018

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

  11. Byron commented on Oct 13, 2018

    @Byron
    Byron
    on Oct 13, 2018
    Member

    Let's do that!

  12. Byron
    closed this as completedon Oct 13, 2018

  13. idbrii commented on Sep 11, 2024

    @idbrii
    idbrii
    on Sep 11, 2024
    Contributor

    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'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    acknowledged

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions