git-deps icon indicating copy to clipboard operation
git-deps copied to clipboard

fix CLI for single revisions

Open robhoes opened this issue 7 years ago • 4 comments

The CLI unconditionally passes all argument to rev-list, even if the argument is a single commit rather than a revision range. This causes the CLI to always analyse the entire git history up to the given commit, rather than just that commit.

Introduced in d601e35f.


This change is Reviewable

robhoes avatar Jan 18 '18 15:01 robhoes

I appears that things like master~3 and mybranch^ still work, because they result in a single commit that is resolved by get_commit in find_dependencies.

I'm not sure about mybranch^!. It is explained (in the git-revisions man page) as specifying a range, but I believe it actually just means the commit at the head of mybranch. That doesn't work anymore (it doesn't work in the GUI either), but then you might as well just write mybranch?

To specify ranges, the current code still handles the .. and ... notations as you would expect.

I think that we want this to behave quite like git show:

  • Given a single commit, it acts on just that commit (unlike git log or git rev-list, which include the commit plus all commits that are reachable from it).
  • Given a range, it acts on all commits in the range.

I think that we could do that by first trying detector.get_commit, and if we catch InvalidCommitish, try GitUtils.rev_list.

robhoes avatar Jan 22 '18 11:01 robhoes

@robhoes commented on 22 Jan 2018, 11:34 GMT:

I appears that things like master~3 and mybranch^ still work, because they result in a single commit that is resolved by get_commit in find_dependencies.

Correct.

I'm not sure about mybranch^!. It is explained (in the git-revisions man page)

I guess you mean the git-rev-parse man page here?

as specifying a range, but I believe it actually just means the commit at the head of mybranch.

Yes, that's a range containing a single commit.

That doesn't work anymore (it doesn't work in the GUI either) but then you might as well just write mybranch?

Ah, I think I see the confusion. I already changed this behaviour in 4caf25ab but that's only the module branch right now, which I haven't yet gotten round to merging into master.

To specify ranges, the current code still handles the .. and ... notations as you would expect.

I think that we want this to behave quite like git show:

  • Given a single commit, it acts on just that commit (unlike git log or git rev-list, which include the commit plus all commits that are reachable from it).
  • Given a range, it acts on all commits in the range.

Yes, exactly - that's what 4caf25ab does.

I think that we could do that by first trying detector.get_commit, and if we catch InvalidCommitish, try GitUtils.rev_list.

It looks like I made the odd decision to fork a git rev-list subprocess to do this, but since it was >=19 months ago, I can't remember why I did that :-/

aspiers avatar Feb 11 '18 11:02 aspiers

@aspiers I'm not sure if you had seen my updated patch. It's quite straightforward and I believe that it does the right thing in all cases.

robhoes avatar Feb 12 '18 10:02 robhoes

Issue was reported in #82.

aspiers avatar Jan 20 '19 23:01 aspiers