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

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.

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

Patrick