From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 Sep 2013 19:48:10 GMT
Subject: Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
Message-ID: <xmqqzjrhom85.fsf@gitster.dls.corp.google.com>
In-Reply-To: <7c478c19da6ee3322ca87e77a90358a30178c286.1379013786.git.john@keeping.me.uk>

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.

Thanks.

> 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);
>  		}