Re: [PATCH] remote.c: Fix overtight refspec validation

> We tightened the refspec validation code in an earlier ef00d15 (Tighten
> refspec processing, 2008-03-17) per my suggestion, but the suggestion was
> misguided to begin with and it broke this usage:
> 
>     $ git push origin HEAD~12:master
> 
> The syntax of push refspecs and fetch refspecs are similar in that they
> are both colon separated LHS and RHS (possibly prefixed with a + to
> force), but the similarity ends there.  For example, LHS in a push refspec
> can be anything that evaluates to a valid object name at runtime (except
> when colon and RHS is missing, or it is a glob), while it must be a
> valid-looking refname in a fetch refspec.  To validate them correctly, the
> caller needs to be able to say which kind of refspecs they are.  It is
> unreasonable to keep a single interface that cannot tell which kind it is
> dealing with, and ask it to behave sensibly.
> 
> This commit separates the parsing of the two into different functions, and
> clarifies the code to implement the parsing proper (i.e. splitting into
> two parts, making sure both sides are wildcard or neither side is).
> 
> This happens to also allow pushing a commit named with the esoteric "look
> for that string" syntax:
> 
>     $ git push ../test.git ':/remote.c: Fix overtight refspec:master'

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>   Daniel Barkalow <barkalow@iabervon.org> writes:
> 
>   > ... (although the 
>   > patch text made it hard to see; we really need some magic to make diff not 
>   > bother to save 4 identifier-free lines and just show it as a complete 
>   > replacement).
> 
>   Yeah, I tend to agree and some people seem to favor "diff -c" for this
>   reason.
> 
>   In any case, here is an updated patch with a better description and
>   necessary tests.  The "validly looking extended SHA-1 expression" check
>   is wholly outside the scope of 1.5.5 regression fix so I did not even
>   attempt to address it, but what this commit fixes needs to be addressed
>   before -rc1.
> 
>  builtin-fetch.c     |    3 +-
>  builtin-send-pack.c |    2 +-
>  remote.c            |  122 ++++++++++++++++++++++++++++++++++-----------------
>  remote.h            |    3 +-
>  t/t5511-refspec.sh  |   67 ++++++++++++++++++++++++++++
>  5 files changed, 153 insertions(+), 44 deletions(-)
>  create mode 100755 t/t5511-refspec.sh
> 
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index b2b9935..a11548c 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -652,5 +652,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  
>  	signal(SIGINT, unlock_pack_on_signal);
>  	atexit(unlock_pack);
> -	return do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);
> +	return do_fetch(transport,
> +			parse_fetch_refspec(ref_nr, refs), ref_nr);
>  }
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 930e0fb..bb9c33a 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -537,7 +537,7 @@ static void verify_remote_names(int nr_heads, const char **heads)
>  	int i;
>  
>  	for (i = 0; i < nr_heads; i++) {
> -		const char *remote = strchr(heads[i], ':');
> +		const char *remote = strrchr(heads[i], ':');
>  
>  		remote = remote ? (remote + 1) : heads[i];
>  		switch (check_ref_format(remote)) {
> diff --git a/remote.c b/remote.c
> index 9700a33..4117bfc 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -393,58 +393,98 @@ static void read_config(void)
>  	alias_all_urls();
>  }
>  
> -struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
> +static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)
>  {
>  	int i;
>  	int st;
>  	struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);
> +
>  	for (i = 0; i < nr_refspec; i++) {
> -		const char *sp, *ep, *gp;
> -		sp = refspec[i];
> -		if (*sp == '+') {
> +		size_t llen, rlen;
> +		int is_glob;
> +		const char *lhs, *rhs;
> +
> +		llen = rlen = is_glob = 0;
> +
> +		lhs = refspec[i];
> +		if (*lhs == '+') {
>  			rs[i].force = 1;
> -			sp++;
> +			lhs++;
>  		}
> -		gp = strstr(sp, "/*");
> -		ep = strchr(sp, ':');
> -		if (gp && ep && gp > ep)
> -			gp = NULL;
> -		if (ep) {
> -			if (ep[1]) {
> -				const char *glob = strstr(ep + 1, "/*");
> -				if (glob && glob[2])
> -					glob = NULL;
> -				if (!glob)
> -					gp = NULL;
> -				if (gp)
> -					rs[i].dst = xstrndup(ep + 1,
> -							     glob - ep - 1);
> -				else
> -					rs[i].dst = xstrdup(ep + 1);
> -			}
> -		} else {
> -			ep = sp + strlen(sp);
> +
> +		rhs = strrchr(lhs, ':');
> +		if (rhs) {
> +			rhs++;
> +			rlen = strlen(rhs);
> +			is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
> +			rs[i].dst = xstrndup(rhs, rlen - is_glob * 2);
>  		}
> -		if (gp && gp + 2 != ep)
> -			gp = NULL;
> -		if (gp) {
> -			rs[i].pattern = 1;
> -			ep = gp;
> +
> +		llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
> +		if (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)))
> +			goto invalid;
> +
> +		if (is_glob) {
> +			llen -= 2;
> +			rlen -= 2;
>  		}
> -		rs[i].src = xstrndup(sp, ep - sp);
> +		rs[i].pattern = is_glob;
> +		rs[i].src = xstrndup(lhs, llen);
>  
> -		if (*rs[i].src) {
> +		/*
> +		 * Do we want to validate LHS?
> +		 *
> +		 * - Fetch refspec must have LHS that names a valid
> +		 *   looking ref (unless it is empty, which defaults to
> +		 *   HEAD).
> +		 *
> +		 * - Push refspec "LHS" without ":RHS" is a synonym for
> +		 *   "LHS:LHS", and LHS must be a valid looking ref
> +		 *   (cannot be empty).
> +		 *
> +		 * - Push refspec "LHS:RHS" that is a glob must be
> +		 *   a valid looking ref (unless it is empty to remove)
> +		 *
> +		 * Hence we check non-empty LHS for fetch, and
> +		 * colonless or glob LHS for push here.
> +		 *
> +		 * Note that push refspec "LHS:RHS" that is not a glob
> +		 * can have any valid extended SHA-1 expression (or
> +		 * "empty" for removal) in LHS, and we cannot check
> +		 * for error until it actually gets used.
> +		 */

>  			st = check_ref_format(rs[i].src);
>  			if (st && st != CHECK_REF_FORMAT_ONELEVEL)
> -				die("Invalid refspec '%s'", refspec[i]);
> +				goto invalid;
>  		}
> -		if (rs[i].dst && *rs[i].dst) {
> +
> +		/*
> +		 * RHS: both push destination and fetch tracking ref
> +		 * must name a valid looking ref, but in fetch, colon
> +		 * followed by emptiness is the same as not having the
> +		 * colon nor RHS.
> +		 */
> +		if (rs[i].dst && (!fetch || *rs[i].dst)) {
>  			st = check_ref_format(rs[i].dst);
>  			if (st && st != CHECK_REF_FORMAT_ONELEVEL)
> -				die("Invalid refspec '%s'", refspec[i]);
> +				goto invalid;
>  		}
>  	}
>  	return rs;
> +
> + invalid:
> +	die("Invalid refspec '%s'", refspec[i]);
> +}