Re: [PATCH v4 2/3] completion: complete tracked paths for 'git diff'

On Fri, Aug 7, 2026 at 8:13 AM Junio C Hamano <[email protected]> wrote:
>
> Elijah Newren <[email protected]> writes:
>
> > On Thu, Aug 6, 2026 at 6:38 PM Junio C Hamano <[email protected]> wrote:
> >>
> >> When completing arguments for 'git diff', _git_diff() delegates to
> >> __git_complete_revlist_file(), which only completes revision
> >> references.  This is good [*], as mixing both revisions and paths in a
> >> single list for the user to pick from is simply too confusing.
> >>
> >> If no reference matches, or if '--' is given, however, _git_diff()
> >> leaves COMPREPLY empty.  Bash then falls back to default filename
> >> completion in $PWD.  This fails when 'git -C <path>' is used because
> >> $PWD is not the target repository.
> >>
> >> Update _git_diff() to use __git_complete_index_file() when '--' is
> >> present, or when revision reference completion yields no matching
> >> candidates, so that tracked paths are offered as candidates.
> >>
> >> This changes behavior even in the case where '-C <there>' is not
> >> used.  The new behavior omits untracked paths from suggestions when
> >> no revs match the prefix but matching tracked paths exist, which is
> >> more useful in the context of 'git diff'.
> >
> > I'm looking forward to using this.  :-)
> >
> > [...]
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index ccd3b2a372..845fd19f70 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -1981,6 +1981,10 @@ _git_diff ()
> >>                 esac
> >>                 __git_complete_revlist_file
> >>         fi
> >> +
> >> +       if [ ${#COMPREPLY[@]} -eq 0 ]; then
> >> +               __git_complete_index_file
> >> +       fi
> >>  }
> >
> > Curious; __git_complete_index_file() is documented as "requires 1
> > argument", but you pass none here.  As far as I can tell, it works
> > anyway, but feels like an accident:
> >
> > 1.   __git_complete_index_file CALLS
> >       __git_index_files "$1" ...
> >       (Here, "$1" == "")
> > 2.   __git_index_files "$1" ... CALLS
> >       __git_ls_files_helper "$root" "$1" ...
> >       (Here, "$1" == "", again)
> > 3.   __git_ls_files_helper "$root" "$1" CALLS
> >       __git -C "$1" -c core.quotePath=false ls-files
> > --exclude-standard $2 -- ...
> >       (Note that $2 is unquoted, and since it's empty, it disappears)
> >
> > It seems like it'd be better to pass an explicit "" to
> > __git_complete_index_file than to implicitly get it.
>
> OK.  It feels a bit strange as an API for the function to insist
> taking one and only one option, which forces the caller to do
>
>         __git_complete_index_file "--cached --others --directory"
>
> when the intention clearly is "we take zero or more options that we
> pass to ls-files", which would have been more obvious if the above
> were written as three separate parameters, but I'll do as Romans in
> the (hopefully small and final) reroll.
>
> Thanks.

Yeah, I don't disagree.  I would be equally happy with an update to
the __git_complete_index_file function to change the comment and
explain what the first argument, if given, means (making it clear that
no arguments are okay); but without either that or having your new
caller pass an argument, the inconsistency between the documentation
and this new caller felt like an issue someone might trip over in the
future.