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

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