Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules

[John Keeping <john@keeping.me.uk>]

On Fri, Sep 13, 2013 at 08:28:24AM +0700, Duy Nguyen wrote:
> On Fri, Sep 13, 2013 at 3:21 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote:
> >> John Keeping <john@keeping.me.uk> writes:
> >>
> >> > This allows us to replace the submodule path trailing slash removal in
> >> > builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to
> >> > parse_pathspec() without changing the behaviour with respect to multiple
> >> > trailing slashes.
> >>
> >> Where does prefix_pathspec()'s input, which could have an unwanted
> >> trailing slash, come from?
> >>
> >> If it is read from some of our internal data structure and known to
> >> have at most one, then this change makes me feel very uneasy to cope
> >> with potentially sloppy end-user input and data generated by ourselves
> >> with the same logic.  It will allow our internal to be sloppy without
> >> forcing us notice and fix that sloppiness.
> >>
> >> If it is coming from an end-user input, then I would not object to
> >> the change, though.
> >
> > I added this in response to Duy's comment on v1 [1].
> >
> > [1] http://article.gmane.org/gmane.comp.version-control.git/234548
> 
> Looks like I add more noise to this thread than anything valuable.
> Yes, once argv goes through parse_pathspec it's normalized so we do
> not need to strip consecutive slashes any more. I'm not entirely sure
> if it also converts Windows '\\' to '/'..

If it goes through normalize_path_copy_len then it does.

Junio, can you please drop the first two patches in this series?  I can
resend the final two unmodified if necessary, but I suspect it's easier
for you to just drop the commits.

> > Looking more closely, this does come from user input (via the argv
> > passed into parse_pathspec) but does (some of the time) go through
> > prefix_path_gently which calls normalize_path_copy_len.
> >
> > It's not immediately clear to me when prefix_pathspec goes through this
> > particular code path, but I think we may be able to drop this (and the
> > previous patch) without affecting the user.