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

On Mon. 10 Aug. 2026 at 14:50, D. Ben Knoble <[email protected]> wrote:
> One other thing, sorry
>
> On Thu, Aug 6, 2026 at 4:36 PM Vincent Mailhol <[email protected]> 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 <[email protected]>
> > ---
> > 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