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

Signed-off-by: Junio C Hamano <redacted>
---

  Daniel Barkalow [off-list ref] 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.
+		 */