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

From: Vincent Mailhol <mailhol@kernel.org>
Date: Aug 7, 2026, 08:09 UTC
Message-ID: <0ea2cce4-2174-4866-9619-d7f74ae5c91f@kernel.org>
In-Reply-To: <anWEcfhdzvNQfskU@pks.im>

On 07/08/2026 at 09:08, Patrick Steinhardt wrote:

Show 30 quoted lines

> 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.
>>
>> Do you foreseen such new parameters?
> 
> 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.

Got it! I kind of mixed subcommands and parameters in my head. My previous answer was totally off topic, sorry.

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

Show 39 quoted lines

>>> 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:
>>
>> ---8<---
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index d313780d8b..786fcb5e16 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2193,6 +2193,15 @@ _git_history ()
>>  		esac
>>  	fi
>>  
>> +	# Subcommands which takes something else than a revision
>> +	case "$subcommand" in
>> +	foo)
>> +		# 'git history foo' take a file first
>> +		__git_complete_index_file "--cached"
>> +		return
>> +		;;
>> +	esac
>> +
>>  	if ! __git_history_has_revision; then
>>  		__git_complete_refs
>>  		return
>> ---8<---
>>
>> 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.
> 
> 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.

Yours sincerely, Vincent Mailhol