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

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

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

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.

> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >  pathspec.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 7c6963b..11b031a 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> >  	item->len = strlen(item->match);
> >  	item->prefix = prefixlen;
> >  
> > -	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
> > -	    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&
> > -	    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> > -	    S_ISGITLINK(active_cache[i]->ce_mode)) {
> > -		item->len--;
> > -		match[item->len] = '\0';
> > +	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) {
> > +		size_t pathlen = item->len;
> > +		while (pathlen > 0 && is_dir_sep(item->match[pathlen - 1]))
> > +			pathlen--;
> > +
> > +		if ((i = cache_name_pos(item->match, pathlen)) >= 0 &&
> > +		    S_ISGITLINK(active_cache[i]->ce_mode)) {
> > +			item->len = pathlen;
> > +			match[item->len] = '\0';
> > +		}
> >  	}
> >  
> >  	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> > @@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> >  			    !is_dir_sep(match[ce_len]) ||
> >  			    memcmp(ce->name, match, ce_len))
> >  				continue;
> > -			if (item->len == ce_len + 1) {
> > -				/* strip trailing slash */
> > +
> > +			while (item->len > 0 && is_dir_sep(match[item->len - 1]))
> >  				item->len--;
> > -				match[item->len] = '\0';
> > -			} else
> > +
> > +			/* strip trailing slash */
> > +			match[item->len] = '\0';
> > +
> > +			if (item->len != ce_len)
> >  				die (_("Pathspec '%s' is in submodule '%.*s'"),
> >  				     elt, ce_len, ce->name);
> >  		}