aspiers / git-deps

git commit dependency analysis tool
GNU General Public License v2.0
298 stars 47 forks source link

fix CLI for single revisions #67

Open robhoes opened 6 years ago

robhoes commented 6 years ago

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

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:

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

aspiers commented 6 years ago

@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 :-/

robhoes commented 6 years ago

@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.

aspiers commented 5 years ago

Issue was reported in #82.