Re: [PATCH] make sure parsed wildcard refspec ends with slash

> A wildcard refspec is internally parsed into a refspec structure with src
> and dst strings.  Many parts of the code assumed that these do not include
> the trailing "/*" when matching the wildcard pattern with an actual ref we
> see at the remote.  What this meant was that we needed to make sure not
> just that the prefix matched, and also that a slash followed the part that
> matched.
> 
> But a codepath that scans the result from ls-remote and finds matching
> refs forgot to check the "matching part must be followed by a slash" rule.
> This resulted in "refs/heads/b1" from the remote side to mistakenly match
> the source side of "refs/heads/b/*:refs/remotes/b/*" refspec.
> 
> Worse, the refspec crafted internally by "git-clone", and a hardcoded
> preparsed refspec that is used to implement "git-fetch --tags", violated
> this "parsed widcard refspec does not end with slash" rule; simply adding
> the "matching part must be followed by a slash" rule then would have
> broken codepaths that use these refspecs.
> 
> This commit changes the rule to require a trailing slash to parsed
> wildcard refspecs.  IOW, "refs/heads/b/*:refs/remotes/b/*" is parsed as
> src = "refs/heads/b/" and dst = "refs/remotes/b/".  This allows us to
> simplify the matching logic because we only need to do a prefixcmp() to
> notice "refs/heads/b/one" matches and "refs/heads/b1" does not.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I have a nagging suspicion that it might be a simpler and cleaner change
> > to change parse_refspec_internal() to keep the trailing slash, instead of
> > dropping it.  Then the check you added is not needed (the trailing slash
> > guarantees that the pattern matches at the hierarchy boundary), neither
> > any of the change in this patch.
> 
> This is the other variant, and it turns out that I was right.  Among the
> 64-18 = 46 new lines, 30 are from the new test file.  Two existing
> "matching part is followed by '/'" tests are removed.

>  remote.c               |   52 +++++++++++++++++++++++++++++++----------------
>  t/t5513-fetch-track.sh |   30 +++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+), 18 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index 0d6020b..f61a3ab 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -427,6 +427,28 @@ static void read_config(void)
>  	alias_all_urls();
>  }
>  
> +/*
> + * We need to make sure the tracking branches are well formed, but a
> + * wildcard refspec in "struct refspec" must have a trailing slash. We
> + * temporarily drop the trailing '/' while calling check_ref_format(),
> + * and put it back.  The caller knows that a CHECK_REF_FORMAT_ONELEVEL
> + * error return is Ok for a wildcard refspec.
> + */
> +static int verify_refname(char *name, int is_glob)
> +{
> +	int result, len = -1;
> +
> +	if (is_glob) {
> +		len = strlen(name);
> +		assert(name[len - 1] == '/');
> +		name[len - 1] = '\0';
> +	}
> +	result = check_ref_format(name);
> +	if (is_glob)
> +		name[len - 1] = '/';
> +	return result;
> +}
> +