From: Thomas Braun <thomas.braun@virtuell-zuhause.de>
To: "SZEDER Gábor" <szeder@ira.uka.de>
Cc: git@vger.kernel.org, peff@peff.net,
Junio C Hamano <gitster@pobox.com>,
Ramkumar Ramachandra <artagnon@gmail.com>,
John Keeping <john@keeping.me.uk>
Subject: Re: [PATCH v4 2/3] completion: add __git_get_option_value helper
Date: Sat, 25 Jun 2016 18:13:01 +0200 (CEST) [thread overview]
Message-ID: <1072705888.508793.1466871181310.JavaMail.open-xchange@app03.ox.hosteurope.de> (raw)
In-Reply-To: <20160610151020.Horde.AfAwgXgKC_jSSpyr60T85sW@webmail.informatik.kit.edu>
> SZEDER Gábor <szeder@ira.uka.de> 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@virtuell-zuhause.de>:
>
> > 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@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.
>
> > +# 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
next prev parent reply other threads:[~2016-06-25 16:13 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-31 23:42 [PATCH 1/2] completion: create variable for untracked file modes Thomas Braun
2016-06-01 4:05 ` Jeff King
2016-06-01 7:02 ` Junio C Hamano
2016-06-01 9:14 ` Thomas Braun
2016-06-01 9:37 ` [PATCH v2 " Thomas Braun
2016-06-01 11:59 ` SZEDER Gábor
2016-06-02 12:19 ` Thomas Braun
[not found] ` <6e722a5fb64b73373ac6450ec9600e98745df29d.1464769152.git.thomas.braun@virtuell-zuhause.de>
2016-06-01 9:37 ` [PATCH v2 2/2] completion: add git status Thomas Braun
2016-06-01 12:15 ` SZEDER Gábor
2016-06-02 15:04 ` Thomas Braun
[not found] ` <9ef8cfd8fb89bcacd123ddbebc12f961a292ef8b.1464879648.git.thomas.braun@virtuell-zuhause.de>
2016-06-02 15:11 ` [PATCH v3 " Thomas Braun
2016-06-02 18:14 ` Junio C Hamano
2016-06-03 15:41 ` Thomas Braun
2016-06-03 16:34 ` Junio C Hamano
2016-06-03 17:17 ` Jeff King
2016-06-03 17:54 ` Junio C Hamano
2016-06-03 18:20 ` Thomas Braun
2016-06-03 18:34 ` [PATCH v4 0/3] support completion for " Thomas Braun
2016-06-03 18:34 ` [PATCH v4 1/3] completion: factor out untracked file modes into a variable Thomas Braun
2016-06-03 18:34 ` [PATCH v4 2/3] completion: add __git_get_option_value helper Thomas Braun
2016-06-10 13:10 ` SZEDER Gábor
2016-06-25 16:13 ` Thomas Braun [this message]
2016-06-03 18:34 ` [PATCH v4 3/3] completion: add git status Thomas Braun
2016-06-06 17:57 ` Junio C Hamano
2016-06-07 7:47 ` Thomas Braun
2016-06-06 18:03 ` [PATCH v4 0/3] support completion for " Junio C Hamano
2016-06-10 10:12 ` [PATCH v5 0/3] completion: add " Thomas Braun
2016-06-10 10:12 ` [PATCH v5 1/3] completion: factor out untracked file modes into a variable Thomas Braun
2016-06-10 10:12 ` [PATCH v5 2/3] completion: add __git_get_option_value helper Thomas Braun
2016-06-10 10:12 ` [PATCH v5 3/3] completion: add git status Thomas Braun
2016-06-10 10:23 ` [PATCH v5 1/3] completion: factor out untracked file modes into a variable Thomas Braun
2016-06-10 10:24 ` [PATCH v5 3/3] completion: add git status Thomas Braun
2016-06-02 15:16 ` [PATCH v3 1/2] completion: factor out untracked file modes into a variable Thomas Braun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1072705888.508793.1466871181310.JavaMail.open-xchange@app03.ox.hosteurope.de \
--to=thomas.braun@virtuell-zuhause.de \
--cc=artagnon@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=john@keeping.me.uk \
--cc=peff@peff.net \
--cc=szeder@ira.uka.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).