| Message ID | 7c7d6de380d1aaa860e86627778bef6c0c78ca08.1618910364.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-completion.bash: fixes on top of 'dl/complete-stash' | expand |
Denton Liu <liu.denton@gmail.com> writes: > In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from > __git_main(), 2021-03-24), the $__git_subcommand_idx variable was > introduced. Naming it after the index of the subcommand is flat-out > wrong as this variable really holds the index of the git comand (e.g. comand -> command > "stash"). > > Rename this variable so that it's obvious it's about git commands. While > we're at it, shorten up its name so that it's still readable without > being a handful to type. As the patch has already written, I won't complain too much, but to many people "git" is a command and "add", "commit" etc. are subcommands of "git", so I do not see git_subcommand_idx so wrong that it needs to be renamed. I do understand that it is a bit too long and it may be easier to type if renamed to say git_cmd_idx, though ;-) > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > contrib/completion/git-completion.bash | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 1dedb14b47..c29c129f87 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1474,12 +1474,12 @@ _git_branch () > > _git_bundle () > { > - local cmd="${words[__git_subcommand_idx+1]}" > + local cmd="${words[__git_cmd_idx+1]}" > case "$cword" in > - $((__git_subcommand_idx+1))) > + $((__git_cmd_idx+1))) > __gitcomp "create list-heads verify unbundle" > ;; > - $((__git_subcommand_idx+2))) > + $((__git_cmd_idx+2))) > # looking for a file > ;; > *) > @@ -1894,7 +1894,7 @@ _git_grep () > esac > > case "$cword,$prev" in > - $((__git_subcommand_idx+1)),*|*,-*) > + $((__git_cmd_idx+1)),*|*,-*) > __git_complete_symbol && return > ;; > esac > @@ -3017,7 +3017,7 @@ _git_stash () > local subcommand="$(__git_find_on_cmdline "$subcommands save")" > > if [ -z "$subcommand" ]; then > - case "$((cword - __git_subcommand_idx)),$cur" in > + case "$((cword - __git_cmd_idx)),$cur" in > *,--*) > __gitcomp_builtin stash_push > ;; > @@ -3058,7 +3058,7 @@ _git_stash () > __gitcomp_builtin stash_branch > ;; > branch,*) > - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then > + if [ $cword -eq $((__git_cmd_idx+2)) ]; then > __git_complete_refs > else > __gitcomp_nl "$(__git stash list \ > @@ -3303,7 +3303,7 @@ _git_worktree () > # be either the 'add' subcommand, the unstuck > # argument of an option (e.g. branch for -b|-B), or > # the path for the new worktree. > - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then > + if [ $cword -eq $((__git_cmd_idx+2)) ]; then > # Right after the 'add' subcommand: have to > # complete the path, so fall back to Bash > # filename completion. > @@ -3327,7 +3327,7 @@ _git_worktree () > __git_complete_worktree_paths > ;; > move,*) > - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then > + if [ $cword -eq $((__git_cmd_idx+2)) ]; then > # The first parameter must be an existing working > # tree to be moved. > __git_complete_worktree_paths > @@ -3395,7 +3395,7 @@ __git_main () > { > local i c=1 command __git_dir __git_repo_path > local __git_C_args C_args_count=0 > - local __git_subcommand_idx > + local __git_cmd_idx > > while [ $c -lt $cword ]; do > i="${words[c]}" > @@ -3426,7 +3426,7 @@ __git_main () > ;; > *) > command="$i" > - __git_subcommand_idx="$c" > + __git_cmd_idx="$c" > break > ;; > esac
On Tue, Apr 20, 2021 at 01:50:19PM -0700, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from > > __git_main(), 2021-03-24), the $__git_subcommand_idx variable was > > introduced. Naming it after the index of the subcommand is flat-out > > wrong as this variable really holds the index of the git comand (e.g. > > comand -> command > > > "stash"). > > > > Rename this variable so that it's obvious it's about git commands. While > > we're at it, shorten up its name so that it's still readable without > > being a handful to type. > > As the patch has already written, I won't complain too much, but to > many people "git" is a command and "add", "commit" etc. are > subcommands of "git", so I do not see git_subcommand_idx so wrong > that it needs to be renamed. I do understand that it is a bit too > long and it may be easier to type if renamed to say git_cmd_idx, > though ;-) The completion functions for git commands having subcommands usually start like this: _git_remote () { local subcommands=" add rename remove set-head set-branches get-url set-url show prune update " local subcommand="$(__git_find_on_cmdline "$subcommands")" if [ -z "$subcommand" ]; then __git_subcommand_idx holds the index of the word "remote", not the index of "add/rename/etc.", so in the context of the completion script that name is misleading.
SZEDER Gábor <szeder.dev@gmail.com> writes: > The completion functions for git commands having subcommands usually > start like this: > > _git_remote () > { > local subcommands=" > add rename remove set-head set-branches > get-url set-url show prune update > " > local subcommand="$(__git_find_on_cmdline "$subcommands")" > if [ -z "$subcommand" ]; then > > __git_subcommand_idx holds the index of the word "remote", not the > index of "add/rename/etc.", so in the context of the completion script > that name is misleading. OK. As subsubcommands would be a mouthful, calling "stash" etc. as "cmd" would be fine.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 1dedb14b47..c29c129f87 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1474,12 +1474,12 @@ _git_branch () _git_bundle () { - local cmd="${words[__git_subcommand_idx+1]}" + local cmd="${words[__git_cmd_idx+1]}" case "$cword" in - $((__git_subcommand_idx+1))) + $((__git_cmd_idx+1))) __gitcomp "create list-heads verify unbundle" ;; - $((__git_subcommand_idx+2))) + $((__git_cmd_idx+2))) # looking for a file ;; *) @@ -1894,7 +1894,7 @@ _git_grep () esac case "$cword,$prev" in - $((__git_subcommand_idx+1)),*|*,-*) + $((__git_cmd_idx+1)),*|*,-*) __git_complete_symbol && return ;; esac @@ -3017,7 +3017,7 @@ _git_stash () local subcommand="$(__git_find_on_cmdline "$subcommands save")" if [ -z "$subcommand" ]; then - case "$((cword - __git_subcommand_idx)),$cur" in + case "$((cword - __git_cmd_idx)),$cur" in *,--*) __gitcomp_builtin stash_push ;; @@ -3058,7 +3058,7 @@ _git_stash () __gitcomp_builtin stash_branch ;; branch,*) - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then + if [ $cword -eq $((__git_cmd_idx+2)) ]; then __git_complete_refs else __gitcomp_nl "$(__git stash list \ @@ -3303,7 +3303,7 @@ _git_worktree () # be either the 'add' subcommand, the unstuck # argument of an option (e.g. branch for -b|-B), or # the path for the new worktree. - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then + if [ $cword -eq $((__git_cmd_idx+2)) ]; then # Right after the 'add' subcommand: have to # complete the path, so fall back to Bash # filename completion. @@ -3327,7 +3327,7 @@ _git_worktree () __git_complete_worktree_paths ;; move,*) - if [ $cword -eq $((__git_subcommand_idx+2)) ]; then + if [ $cword -eq $((__git_cmd_idx+2)) ]; then # The first parameter must be an existing working # tree to be moved. __git_complete_worktree_paths @@ -3395,7 +3395,7 @@ __git_main () { local i c=1 command __git_dir __git_repo_path local __git_C_args C_args_count=0 - local __git_subcommand_idx + local __git_cmd_idx while [ $c -lt $cword ]; do i="${words[c]}" @@ -3426,7 +3426,7 @@ __git_main () ;; *) command="$i" - __git_subcommand_idx="$c" + __git_cmd_idx="$c" break ;; esac
In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from __git_main(), 2021-03-24), the $__git_subcommand_idx variable was introduced. Naming it after the index of the subcommand is flat-out wrong as this variable really holds the index of the git comand (e.g. "stash"). Rename this variable so that it's obvious it's about git commands. While we're at it, shorten up its name so that it's still readable without being a handful to type. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- contrib/completion/git-completion.bash | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)