Re: [PATCH v4 2/3] completion: complete tracked paths for 'git diff'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 7, 2026, 15:13 UTC
- Message-ID
- <xmqqldaiezgd.fsf@gitster.g>
- In-Reply-To
- <CABPp-BEAtpT208afwSNoBbR-Nowss8OsLsL8ynETuBfN_xvWag@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 55 quoted lines
> On Thu, Aug 6, 2026 at 6:38 PM Junio C Hamano <gitster@pobox.com> 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.