Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add completion for revision range #1975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zldrobit
Copy link

This PR adds the command line completion for git revision range, e.g.:

:Gclog master..origin/feature
:Gclog master...origin/master

Comment on lines 2438 to 2441
let [pre, base] = s:SplitRevRange(base, '\.\.\.')
if empty(pre)
let [pre, base] = s:SplitRevRange(base, '\([^:]\|^\)\.\.')
endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could do this with one pattern: '^[^:]*\zs.\.\.\='. This would also avoid splitting on .. in hash:a/../b.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion. I didn't know \zs and \ze before. Though '^[^:]*\zs.\.\.\=' avoids splitting on .. in hash:a/../b, some range expression are not split as expected. Now I use '\([^:]|^\)\zs\.\.\.\=\ze\([^/]|$\)' for splitting now, and some examples are illustrated below,

prefix with '^[^:]*\zs.\.\.\=' prefix with '\([^:]|^\)\zs\.\.\.\=\ze\([^/]|$\)' expected
hash:a/../b empty string empty string empty string
hash:../a hash:.. empty string empty string
hash:a/..hash2 empty string hash:a/.. hash:a/..

For simplicity, I pushed a new commit with push -f.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect hash:a/..hash2 to return an empty string. This is just a blob in commit hash with the unusual name of ..hash2 in directory a/, no?

For simplicity, I pushed a new commit with push -f.

Always preferred, thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood the hash:a/..hash2, and I changed the pattern a little bit ('\([^:]|^\)\zs\.\.\.\=\ze\([^/]|$\)' -> '\([^:/]|^\)\zs\.\.\.\=\ze\([^/\.]|$\)') to accommodate this situation. To verify the new regular expression, previous examples are updated, and more examples are added to the table as below,

prefix with '^[^:]*\zs.\.\.\=' prefix with '\([^:/]|^\)\zs\.\.\.\=\ze\([^/\.]|$\)' expected
hash:a/../b empty string empty string empty string
hash:../a hash:.. empty string empty string
hash:a/..hash2 empty string empty string empty string
hash:a/..b..hash2 empty string hash:a/..b.. hash:a/..b..
hash.../a hash... empty string empty string

Hope this could make completion more convenient.

Copy link
Owner

@tpope tpope Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add hash:a..b—another unusual filename—to the list. I think you might want '^[^:]*\zs/\@<!.\.\.\=/\@!'. There's 2 new Vim regexp atoms for you to add to your toolbelt.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are two powerful atoms, thanks. I updated the table with hash:a..b, and the situation is more complicated. It depends on whether a..b exists as a file. If it is the case, it should be avoided to split on ...

prefix with '^[^:]*\zs\/\@<!\.\.\.\=\/\@!' prefix with '[:/]\@<!\.\.\.\=[/\.]\@!' expected
hash:a/../b empty string empty string empty string
hash:../a empty string empty string empty string
hash:a/..hash2 empty string empty string empty string
hash:a/..b..hash2 empty string hash:a/..b.. hash:a/..b..
hash.../a hash.. empty string empty string
hash:a..b (a..b exists) empty string hash:a.. empty string
hash:a..b (a..b does not exist) empty string hash:a.. hash:a..

I found that git-completion in bash could distinguish the two situations of hash:a..b, so I'll dig into that script to see if I could find any method to improve this PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand these "two situations". There is no case when hash:a..b should be interpreted as a commit range because there's no case when hash:a should be interpreted as a commit. I can't reproduce with the Bash completion either, but even if I could, I would consider that a bug, because it makes no sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tpope I agree with you. hash:a..b shouldn't be interpreted as a commit range, but it is a valid prefix for hash:a..b:c (file comparing) if a is a file and b is a commit. :Gclog completion process goes into fugitive#LogComplete() -> fugitive#CompleteObject() and fugitive#CompletePath() sequentially. :G diff has a similar call stack for completion process (fugitive#Complete() -> fugitive#CompleteObject() -> fugitive#CompletePath()). Since both commands invoke fugitive#CompleteObject(), It is hard to distinguish between hash:a..b (a..b is a file) and hash:a..hash2 (a is a file and hash2 is a commit), unless the files are scanned through (e.g., with git ls-files).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash:a..b shouldn't be interpreted as a commit range, but it is a valid prefix for hash:a..b:c (file comparing) if a is a file and b is a commit.

I was unaware of this blob vs blob range. I've never used it and I've never seen anyone else use it. My vote would be to ignore it.

Copy link
Author

@zldrobit zldrobit May 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With vim-fugitive, it's a rare case to use this blob vs blob range, which I used before. Blob vs blob range has a lower priority than hash:a..b (a..b is a file), and one could use git diff hash..hash2 -- file for file comparison. So, I keep the revision range not to be split when encountering hash:a..b with '^[^:]*\/\@<!\.\.\.\=[\/\.]\@!'. I simplified the table of examples as below, and the behaviors are as expected.

prefix with '^[^:]*\/\@<!\.\.\.\=[\/\.]\@! expected
hash:a/../b empty string empty string
hash:../a empty string empty string
hash:a/..hash2 empty string empty string
hash.../a empty string empty string
hash:a..b empty string empty string
hash..a hash.. hash..
hash...a hash... hash...
remote/branch..remote2 remote/branch.. remote/branch..

EDIT:
Add remote/branch..remote2 in the example table.

@zldrobit zldrobit force-pushed the add-rev-range-comp branch 4 times, most recently from 5752cf4 to cc39ef1 Compare April 21, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants