> SZEDER Gábor <szeder@xxxxxxxxxx> hat am 10. Juni 2016 um 15:10 geschrieben:
>
> 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.
Hi Gábor,
thanks for your comments.
I plan to send a reroll in the near future adressing your remarks.
Bye,
Thomas
> Quoting Thomas Braun <thomas.braun@xxxxxxxxxxxxxxxxxxx>:
>
> > 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 :)
>
> > Signed-off-by: Thomas Braun <thomas.braun@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
>
> > +# 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; do
>
> Without 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 ...
>
> > + 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.
>
> > +}
> > +
> > __git_has_doubledash ()
> > {
> > local c=1
> > --
> > 2.8.3.windows.1
>
>
> However, 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
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html