Re: [PATCH v3 0/2] completion: hide dotfiles for selected path completion

[Junio C Hamano <gitster@pobox.com>]

"Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The completion helper for index paths uses git ls-files rather than shell
> filename completion. As a result, leading-dot paths such as a tracked
> .gitignore were offered even when the user had not started the path with ..
>
> Hide leading-dot path components for git rm, git mv, and git ls-files when
> completing an empty path component. Explicit dot completion is still
> preserved, so git rm . can still complete .gitignore.
>
> This removes the existing TODO expectations in t/t9902-completion.sh and
> adds coverage for explicit dot completion.

OK.

> Validation:
>
>  * git diff --check -- contrib/completion/git-completion.bash
>    t/t9902-completion.sh
>  * bash -n contrib/completion/git-completion.bash
>  * ./t9902-completion.sh

I am not sure what you wanted to say with these lines.  If you did
the above to build confidence that your patch works, that would be
great.  Or are you telling readers to do these things and when they
do not see any issues consider your patch perfect?

What is missing around here in this cover letter is a description of
how this iteration is different from the previous one.  And ...

> Zakariyah Ali (2):
>   completion: hide dotfiles for selected path completion
>   completion: hide dotfiles by default for path completion
>
>  contrib/completion/git-completion.bash | 53 +++++++++++++++-----------
>  t/t9902-completion.sh                  | 19 ++++-----
>  2 files changed, 40 insertions(+), 32 deletions(-)
>
>
> base-commit: 9b7fa37559a1b95ee32e32858b0d038b4cf583e5
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2311%2Falibaba0010%2Fcompletion-hide-dotfiles-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2311/alibaba0010/completion-hide-dotfiles-v3
> Pull-Request: https://github.com/git/git/pull/2311
>
> Range-diff vs v2:
>
>  1:  056e239e06 = 1:  056e239e06 completion: hide dotfiles for selected path completion
>  -:  ---------- > 2:  7482ee4645 completion: hide dotfiles by default for path completion

... I find this range diff very troubling.  If we look at patch 2,
it seems that it redoes some part of what is done in patch 1 saying
"oops that was wrong, so let's do it better this time".  Such a
drunken-mans' walk that goes in one direction in an earlier step,
only to be corrected to move to a different course, is now how we
want a new topic to be presented.

The end result may be much easier to read, mostly thanks to updated
loop in the awk script, so if we really want to pretend this as two
patches for "small pieces are easier to digest" value, perhaps have
[PATCH 1/2] that updates the awk script (without doing anything
related to hide-dotfiles theme) to make it easier to read by not
having multiple "print pfx p" in it, and then build on top of that
improved base, have [PATCH 2/2] that adds the support to hide
dotfiles, perhaps?

Since the initial iteration was quite a while ago, I no longer
remember the details of the review I gave, but I recall having hard
time telling which callers of the complete-index-file helper hide
dotfiles from their output and which callers do not hide them, and
how the patch decided to choose which ones should and should not
hide.  Has it been improved and if so how?  That is something we
expect the cover letter to tell, too.

Thanks.