Re: [PATCH] completion: add 'git history' subcommands

Hi all,

I agree with Patrick's review below, this looks good to me! One note…

On Wed, Aug 5, 2026 at 2:25 AM Patrick Steinhardt <[email protected]> wrote:
>
> On Tue, Aug 04, 2026 at 09:56:32PM +0200, Vincent Mailhol wrote:
> > Use the parse-options completion helpers for the "git history"
> > subcommands and their options. Complete positional arguments as
> > revisions, and add coverage for each kind of completion.
>
> Ah, great! I wanted to write shell completion for git-history(1) for a
> while but never really found the time to actually do it.
>
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index e875787710..f10813c8d7 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -2137,6 +2137,30 @@ _git_help ()
> >       fi
> >  }
> >
> > +_git_history ()
> > +{
> > +     local subcommands subcommand
> > +
> > +     __git_resolve_builtins "history"
> > +
> > +     subcommands="$___git_resolved_builtins"
> > +     subcommand="$(__git_find_subcommand "$subcommands")"
> > +
> > +     if [ -z "$subcommand" ]; then
> > +             __gitcomp "$subcommands"
> > +             return
> > +     fi
>
> Okay. We first try to figure out whether there is any subcommand passed
> by the user already. If not, we complete available subcommands.
>
> > +     case "$cur" in
> > +     --*)
> > +             __gitcomp_builtin "history_$subcommand"
> > +             ;;
> > +     *)
> > +             __git_complete_refs
> > +             ;;
> > +     esac
> > +}
>
> Otherwise we try to either complete available options if we see a
> leading "--", or alternatively we complete references. This works well
> for "drop", "fixup" and "reword".
>
> The one command where this falls flat a bit is `git history split`.
> While the first non-option argument is indeed a reference, subsequent
> arguments are pathspecs. So ideally, we'd notice that we already have a
> reference there and, if so, complete file paths.

…here: I think it's probably going to look a bit like what _git_reflog
and _git_config do, checking the subcommand and using somewhat more
specific completion in that case.

BTW, I'm also reminded of <[email protected]>, where Junio
suggested we devise a way to improve _git_stash. Looking at it again
with some context, I bet we can reuse the __git_resolve_builtins
pattern there, too. Not for this patch, of course, just thinking
aloud!

> But that being said, I think this is a good-enough first iteration and a
> strict improvement over the status quo -- we don't have to be perfect
> right from the start. So if you want to also make that case work then
> great, but I won't insist on it.
>
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index 9ae3c48ebd..08ecf682ed 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -3107,6 +3107,23 @@ test_expect_success 'git clone --config= - value' '
> >       EOF
> >  '
> >
> > +test_expect_success 'git history subcommands' '
> > +     test_completion "git history " <<-\EOF
> > +     drop Z
> > +     fixup Z
> > +     reword Z
> > +     split Z
> > +     EOF
> > +'
>
> This will cause conflicts with "seen", as there's a new upcoming
> "squash" command that's currently cooking there. That's fine though,
> nothing you can do about that.
>
> Thanks!
>
> Patrick

Thanks!

-- 
D. Ben Knoble