From: Vincent Mailhol <mailhol@kernel.org>
Date: Fri, 07 Aug 2026 06:44:41 GMT
Subject: Re: [PATCH v2 1/4] completion: add 'git history' subcommands
Message-ID: <e894cf4e-7df2-489a-a596-96f1d4d95dc0@kernel.org>
In-Reply-To: <anV7cHblfmGvbl-e@pks.im>

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?

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

> That'd still be a huge win compared to the status quo, and if we really
> care about making this work properly we can still iterate.

Thanks!


Yours sincerely,
Vincent Mailhol