From: Vincent Mailhol <mailhol@kernel.org>
Date: Thu, 13 Aug 2026 08:20:00 GMT
Subject: Re: [PATCH v2 2/4] completion: complete 'git history --empty' values
Message-ID: <CAMZ6RqLAYMSwNK=w=Xh+O==46eQCS=wFgBoUEOtQoBbLrBqd_A@mail.gmail.com>
In-Reply-To: <CALnO6CAssyDe7uOK+G8eZPzu1S6iyn8EiSQGqUHtWgdPcD65xw@mail.gmail.com>

On Mon. 10 Aug. 2026 at 14:50, D. Ben Knoble <ben.knoble@gmail.com> wrote:
> One other thing, sorry
>
> On Thu, Aug 6, 2026 at 4:36 PM Vincent Mailhol <mailhol@kernel.org> wrote:
> >
> > The "--empty" option accepts "drop", "keep", or "abort" for the "drop"
> > and "fixup" subcommands. Complete these values.
> >
> > Although the synopsis only documents the:
> >
> >   --empty=<value>
> >
> > form, parse-options also accepts the value as a separate argument:
> >
> >   --empty <value>
> >
> > Support both forms to follow the parser.
> >
> > Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> > ---
> > Changes in v2:
> >
> >   - New patch.
> > ---
> >  contrib/completion/git-completion.bash | 13 +++++++++++--
> >  t/t9902-completion.sh                  |  5 ++++-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 7372e2919b..fe5223b8ec 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -2171,8 +2171,17 @@ _git_history ()
> >         fi
> >
> >         if ! __git_has_doubledash; then
> > -               case "$cur" in
> > -               --*)
> > +               case "$prev,$cur" in
> > +               --empty,*|*,--empty=*)
> > +                       case "$subcommand" in
> > +                       drop|fixup)
>
> This feels a bit "inside out" to me, especially when reading the other
> completions. I think the usual pattern is to check the subcommand
> first and dispatch if necessary. Thoughts?

The motivation is to have a single:

  case "$cur" in

statement.

After dropping support for the separated option-value form, this is
what the code looks like if we check the subcommand first and the
option second:

        if ! __git_has_doubledash; then
                case "$subcommand" in
                drop|fixup)
                        case "$cur" in
                        --empty=*)
                                __gitcomp "drop keep abort" "" \
                                        "${cur##--empty=}"
                                return
                                ;;
                        esac
                        ;;
                esac

                case "$cur" in
                --update-refs=*)
                        __gitcomp "branches head" "" \
                                "${cur##--update-refs=}"
                        return
                        ;;
                --*)
                        __gitcomp_builtin "history_$subcommand"
                        return
                        ;;
                esac
        fi

See the repeated 'case "$cur" in'. Note that it is not possible to
have a wild card *) in the first switch case unless the
--update-refs=*) dispatch gets duplicated. In the end, by using this
approach, one part of the code will need to get duplicated.

On the contrary, by dispatching the option first and the subcommand
second like this:

        if ! __git_has_doubledash; then
                case "$cur" in
                --empty=*)
                        case "$subcommand" in
                        drop|fixup)
                                __gitcomp "drop keep abort" "" \
                                        "${cur##--empty=}"
                                ;;
                        esac
                        return
                        ;;
                --update-refs=*)
                        __gitcomp "branches head" "" \
                                "${cur##--update-refs=}"
                        return
                        ;;
                --*)
                        __gitcomp_builtin "history_$subcommand"
                        return
                        ;;
                esac
        fi

we do not see the conflict and do not need to repeat any of the switch cases.

> > +                               __gitcomp "drop keep abort" "" \
> > +                                       "${cur##--empty=}"
> > +                               return
> > +                               ;;
> > +                       esac
> > +                       ;;
> > +               *,--*)
> >                         __gitcomp_builtin "history_$subcommand"
> >                         return
> >                         ;;
> [snip]


Yours sincerely,
Vincent Mailhol