| Message ID | pull.1227.git.git.1647032857097.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tab completion of filenames for 'git restore' | expand |
"David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: David Cantrell <david@cantrell.org.uk> > > If no --args are present after 'git restore' it assumes that you want > to tab-complete one of the files with uncommitted changes > > Signed-off-by: David Cantrell <david@cantrell.org.uk> > --- > Improved bash tab completion for 'git restore' - adds support for > auto-completing filenames > > This adds tab-completion of filenames to the bash completions for git > restore. Two questions - "restore" is a castrated half "checkout"; shouldn't the latter also be getting the same feature? - is "complete_index_file --committable" the right thing to use? It boils down to running "diff-index HEAD", which means path with differences from the HEAD commit is listed. By default "restore" checks out the contents of the given path from the index to the working tree, so after "edit F && git add F", "diff-index HEAD" may show F in its output (i.e. F is "committable"), but "restore F" would be a no-op. Which feels a bit iffy. Modelling it after "git add" completion, where we look for paths that are different between the index and the working tree, feels more appropriate, but I haven't thought things through. > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1227%2FDrHyde%2Ffilename-completion-for-git-restore-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1227/DrHyde/filename-completion-for-git-restore-v1 > Pull-Request: https://github.com/git/git/pull/1227 > > contrib/completion/git-completion.bash | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 49a328aa8a4..7ccad8ff4b1 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -2883,14 +2883,21 @@ _git_restore () > case "$cur" in > --conflict=*) > __gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}" > + return > ;; > --source=*) > __git_complete_refs --cur="${cur##--source=}" > + return > ;; > --*) > __gitcomp_builtin restore > + return > ;; > esac > + > + if __git rev-parse --verify --quiet HEAD >/dev/null; then > + __git_complete_index_file "--committable" > + fi > } Do you need to sprinkle return's? Instead you could just add another case arm, like case "$cur" in --conflict=*) ... all the existing code ... --*) __gitcomp_builtin restore ;; + *) + ... whatever you want to do when + ... $cur is not a --dashed-option + ;; esac
On 13/03/2022 06:45, Junio C Hamano wrote: > "David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes: >> Improved bash tab completion for 'git restore' - adds support for >> auto-completing filenames >> >> This adds tab-completion of filenames to the bash completions for git >> restore. > Two questions > > - "restore" is a castrated half "checkout"; shouldn't the latter > also be getting the same feature? `git checkout <tab>` already completes to a list of branches and tags, which is I think more useful in that case. > - is "complete_index_file --committable" the right thing to use? > > It boils down to running "diff-index HEAD", which means path with > differences from the HEAD commit is listed. By default "restore" > checks out the contents of the given path from the index to the > working tree, so after "edit F && git add F", "diff-index HEAD" > may show F in its output (i.e. F is "committable"), but "restore > F" would be a no-op. Which feels a bit iffy. I'd not thought of that. --modified is better. >> @@ -2883,14 +2883,21 @@ _git_restore () >> case "$cur" in >> --conflict=*) >> __gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}" >> + return >> ;; >> --source=*) >> __git_complete_refs --cur="${cur##--source=}" >> + return >> ;; >> ... > Do you need to sprinkle return's? Instead you could just add > another case arm, like > > + *) > + ... whatever you want to do when > + ... $cur is not a --dashed-option > + ;; Liberal sprinkling of return like that seems to be the norm for the rest of the file so I stuck with it.
David Cantrell <david@cantrell.org.uk> writes: >>> @@ -2883,14 +2883,21 @@ _git_restore () >>> case "$cur" in >>> --conflict=*) >>> __gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}" >>> + return >>> ;; >>> --source=*) >>> __git_complete_refs --cur="${cur##--source=}" >>> + return >>> ;; >>> ... >> Do you need to sprinkle return's? Instead you could just add >> another case arm, like >> + *) >> + ... whatever you want to do when >> + ... $cur is not a --dashed-option >> + ;; > > Liberal sprinkling of return like that seems to be the norm for the > rest of the file so I stuck with it. An existing bad practice is not a good excuse to spread it even more, though.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 49a328aa8a4..7ccad8ff4b1 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2883,14 +2883,21 @@ _git_restore () case "$cur" in --conflict=*) __gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}" + return ;; --source=*) __git_complete_refs --cur="${cur##--source=}" + return ;; --*) __gitcomp_builtin restore + return ;; esac + + if __git rev-parse --verify --quiet HEAD >/dev/null; then + __git_complete_index_file "--committable" + fi } __git_revert_inprogress_options=$__git_sequencer_inprogress_options