Re: [PATCH v2 2/4] completion: complete 'git history --empty' values

From: Vincent Mailhol <mailhol@kernel.org>
Date: Aug 13, 2026, 08:20 UTC
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:

Show 45 quoted lines

> 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.

Show 11 quoted lines

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

Yours sincerely, Vincent Mailhol