fix CLI for single revisions

[robhoes]

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

---

This change is 

[aspiers]

Firstly thanks a lot for the suggestion! Yes it does seem that this can be improved. I'm not 100% convinced by your exact approach here, since IIUC this would break passing in arguments like master~3 or mybranch^!. It seems that the web GUI code does an additional call to detector.get_commit to handle those, so maybe also reusing that logic is the answer? In which case, a new function should probably be refactored out in order to avoid duplicating code.

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

[aspiers]

@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 4caf25a 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 4caf25a 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]

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

Issue was reported in #82.