Re: [PATCH v4 2/3] completion: add __git_get_option_value helper
Hallo Thomas,
I saw v5 hit my mailbox while writing this. I glanced it over and it seems my comments here apply to that version as well.
Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:
Show 6 quoted lines
> This function allows to search the commmand line and config
> files for an option, long and short, with mandatory value.
>
> The function would return e.g. for the command line
> "git status -uno --untracked-files=all" the result
> "all" regardless of the config option.
Wow, regarding my earlier remark about bonus points: I didn't realize that there were so many bonus point to give away :)
Show 19 quoted lines
> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
> contrib/completion/git-completion.bash | 44
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index addea89..4bd17aa 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -803,6 +803,50 @@ __git_find_on_cmdline ()
> done
> }
>
> +# Echo the value of an option set on the command line or config
> +#
> +# $1: short option name
> +# $2: long option name including =
I'm not sure about requiring the '=', the function could just append it as necessary. More on this below.
> +# $3: list of possible values
> +# $4: config string (optional)
I don't understand why the list of possible values is necessary.
This function will be called when the caller wants to take different actions based on different values, so the caller will process the function's output with a case statement or an if-else chain, both of which would be perfectly capable to ignore whatever invalid value the user might have specified. Therefore, I think this function doesn't need the list of possible values, it should just return whatever value it found after the option.
Show 16 quoted lines
> +# example:
> +# result="$(__git_get_option_value "-d" "--do-something="\
> +# "yes no" "core.doSomething")"
> +#
> +# result is then either empty (no option set) or "yes" or "no"
> +#
> +# __git_get_option_value requires 3 arguments
> +__git_get_option_value ()
> +{
> + local c short_opt long_opt val
> + local result= values config_key word
> +
> + short_opt="$1"
> + long_opt="$2"
> + values="$3"
> + config_key="$4"These can be assigned when the variables are declared, saving a couple of lines.
> + ((c = $cword - 1))
> + while [ $c -ge 0 ]; do
Searching from the end of the command line, so even if someone were to do a 'git status -uall -unormal -uno <TAB>', this would still do the right thing. Good!
However ;) Just for fun imagine following:
$ >-uno
$ git status -- -uno <TAB>'git status' treats that '-uno' after the doubledash as a filename, but this function interprets it as an option, and on the subsequent TAB the completion script won't list untracked files.
I'm tempted to say that this is such a pathological corner case that it doesn't worth worrying about.
> + word="${words[c]}"
> + for val in $values; doWithout the possible values argument this inner loop could go away.
> + if [ "$short_opt$val" = "$word" ]
> + || [ "$long_opt$val" = "$word" ]; then
> + result="$val"
> + break 2
You could just 'echo "$val"' or rather ${word#$short_opt} and return here ...
Show 6 quoted lines
> + fi
> + done
> + ((c--))
> + done
> +
> + if [ -n "$config_key" ] && [ -z "$result" ]; then
... and that would make the second condition unnecessary here ...
> + result="$(git --git-dir="$(__gitdir)" config "$config_key")"
... and this could just be a simple 'git config' execution, without command substitution ...
> + fi
> +
> + echo "$result"
... and this echo could go away as well.
Show 7 quoted lines
> +}
> +
> __git_has_doubledash ()
> {
> local c=1
> --
> 2.8.3.windows.1However, I'm not sure we need or want this helper function _at the moment_. Yes, in general helper functions are good, and in this case it makes _git_status() easier to follow, but it has some drawbacks, too:
- It has a single callsite: the upcoming _git_status(). No other
existing case springs to mind where it could be used, i.e. where
different values of an option would require different actions from
the completion script. Maybe we'll have one in the future, maybe
not. - This function works only with the "stuck" form of options, i.e.
'--opt=val' or '-oval', which is mostly sufficient in this case,
because 'git status' understands only this form. However, it
doesn't work with "unstuck" options, i.e. '--opt val' or '-o val'.
In many cases git supports only this "unstuck" form, and there are
many cases where it supports both for a given option. We can't know
which form a future callsite might need, but requiring the '=' as
part of the long option seems to paint us into a corner. - I wrote "mostly sufficient" above, because 'git status' does accept
a valueless '-u|--untracked-files' option, too, e.g.: $ git config status.showUntrackedFiles no
$ git status --untracked-files lists untracked files, therefore the completion script should list
them as well. Your function can't cope with this case, and I'm not
sure how it and its caller could differentiate between the presence
of such a valueless option and no option at all. Perhaps with an
additional optional function parameter holding the default value
that should be echo-ed when a valueless option is encountered.If this function were not a function but its logic were embedded into _git_status(), then we wouldn't have to spend any effort _now_ to come up with a proper calling convention that can cope with stuck vs. unstuck vs. both forms of options and with valueless options. We would deal with all that and the necessary refactorization when (or if ever) there's a second potential callsite. Embedding into _git_status() would give you more freedom to deal with the valueless '-u' option, too. If embedded, some of my in-code comments wouldn't apply anymore, of course.
I'm in favor of crossing the bridge when we get there.
Gábor
Thomas Braun
