Re: [PATCH v2 1/4] completion: add 'git history' subcommands

On Fri, Aug 7, 2026 at 4:13 AM Vincent Mailhol <[email protected]> wrote:
>
> On 07/08/2026 at 09:08, Patrick Steinhardt wrote:
> > On Fri, Aug 07, 2026 at 08:44:41AM +0200, Vincent Mailhol wrote:
> >> On 07/08/2026 at 08:30, Patrick Steinhardt wrote:
> >>> On Thu, Aug 06, 2026 at 10:27:36PM +0200, Vincent Mailhol wrote:
> >>>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >>>> index e875787710..7372e2919b 100644
> >>>> --- a/contrib/completion/git-completion.bash
> >>>> +++ b/contrib/completion/git-completion.bash
> >>>> @@ -2137,6 +2137,54 @@ _git_help ()
> >>>>    fi
> >>>>  }
> >>>>
> >>>> +__git_history_has_revision ()
> >>>> +{
> >>>> +  local i
> >>>> +
> >>>> +  for ((i = __git_cmd_idx + 2; i < cword; i++)); do
> >>>> +          case "${words[i]}" in
> >>>> +          --empty|--update-refs)
> >>>> +                  ((i++))
> >>>> +                  ;;
> >>>
> >>> This will unfortunately be quite a pain to maintain going forward, as we
> >>> now have to be aware of updating this site every single time we add a
> >>> new option that accepts a parameter.
[snip]
> > Yes, I'm very sure we'll gain more parameters for those commands. Commit
> > signing, sign-offs, handling of notes are all things that are currently
> > being discussed, and they likely will require new options.
[snip]
>
> For the new parameters, indeed. The issue is that these options accept
> two syntax:
>
>   --empty=<value>
>
> or
>
>   --empty <value>
>
> The first one falls under the '-*)' switch case anyway, so if you do a
>
>   git history fix --new-option=foo <TAB>
>
> the __git_history_has_revision will handle it properly. If you do:
>
>   git history fix --new-option=<TAB>
>
> you just get no completion until the code is modified to teach what are
> the correct value for --new-option. This is acceptable in term of
> maintainability.
>
> If you do:
>
>   git history fix --new-option <TAB>
>
> then __git_history_has_revision will assume that --new-option is a
> toggle parameter which takes no value and will incorrectly complete it
> with a reference.
>
> Finally, if you do a:
>
>   git history fix --new-option value <TAB>
>
> then the value is interpreted as a reference and the <TAB> gives no
> completion.
>
> For a
>
>   git history fix --gpg-sign
>
> this is mostly OK. Assuming the new --gpg-sign works identically as the
> git rebase option, the --gpg-sign value is optional and default the the
> committer identity. So in most of the cases, the user will not give a
> value and will correctly get the reference completion when doing:
>
>   git history fix --gpg-sign <TAB>
>
> So the only case where we are screwed is if the option takes an argument
> *and* the user specify it as --new-option (without the final '='). In
> that case, the damage is still not huge. I expect most of the users to
> pass option with the final '='.

[later]

> > I'm not really concerned about new subcommands for now, true. But
> > hardcoding the parameters as we do above feels error prone to me and
> > will very likely diverge as the command evolves.
>
> I think that there are two options:
>
>   1. What I did, which work great today and will start to diverge the
>      day we add more arguments which takes a value as you highlighted.
>
>   2. Ignore the '--argument <value>' syntax and only complete the
>      '--argument=<value>'.
>
> Point 2. will consistently give incorrect results when doing:
>
>   git history fix --new-option value <TAB>
>
> but is easier to maintain. And the '--argument <value>' syntax isn't
> covered in the manpages anyway, so this option is just a "we implement
> the manpages and that's it!" approach.
>
> My preference goes slightly to 1., but I am OK to send a v3 with
> option 2.

- The manuals (gitcli, especially) recommend the stuck form (-oArg,
--long-opt=Arg)
- Completion code that I'm aware of completes the string "--long-opt="

So I suspect most folks using completion will end up with the stuck
form. If we want to support the unstuck form, I'm ok with that
(Vincent's (1)). It seems simpler for now to go with (2), which aligns
with the rest of the codebase, and wait to see if anyone complains
though.

Switching topics:

> >>> I don't really have a good idea for how to fix that reliably though, I
> >>> have to admit. Maybe we should just mostly ignore this edge case and
> >>> always complete references, unless we have seen a `--`? That can be
> >>> checked rather easily via `__git_hash_doubledash`.
> >>
> >> My toughs are that if such a special case ever surface, we can just
> >> dispatch it earlier before we check for the
> >> __git_history_has_revision, like this:
[snip]
> >>
> >> This seems reasonable to me. Once we know what this mysterious new
> >> command would be, maybe we can find a smarter and more tailored
> >> solution, but at the moment, I would not call this a blocker.

This is similar to what we do stash and a few other
subcommand-commands, where we need to dispatch a bit differently. I
think trying to assume all git-history commands will have the same
shape is both pleasant (consistent interface!) and unlikely to hold up
(something will diverge somewhere).

-- 
D. Ben Knoble