Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 7, 2013, 00:04 UTC
Message-ID: <20131207000437.GP29959@google.com>
In-Reply-To: <20131206220751.GB25620@sigill.intra.peff.net>

Jeff King wrote:

> As an aside, this is one of those places where C's string functions do
> gross things with const.

Yes, yuck.

The fundamental grossness is that argv is semantically char ** (assuming this doesn't segfault) but passed around as const char **. I wonder why we don't use the same trick in git_extract_argv0_path and when handling STRIP_EXTENSION in git.c.

Alternatively, try_difference could create a new buffer for its own use. Originally (v0.99~273, 2005-06-13) this dotdot handling code was pretty simple:

	for (i = 1; i < argc; i++) {
		char *arg = argv[i];

...

		dotdot = strstr(arg, "..");
		if (dotdot) {
			unsigned char end[20];
			char *n = dotdot+2;
			*dotdot = 0;
			if (!get_sha1(arg, sha1)) {
				if (!*n)
					n = "HEAD";
				if (!get_sha1(n, end)) {
					printf("%s\n", sha1_to_hex(end));
					printf("^%s\n", sha1_to_hex(sha1));
					continue;
				}
			}
			*dotdot = '.';
		}
		printf("%s\n", arg);
	}

The 'continue' without making the argument right again was justified because we were done with the argument.

Now it is still not much more complicated --- the function is logically just trying to parse the argument into a "this" and "next" part and print them.

Something like the following (untested). Or if the allocation churn is too much, it could use a static strbuf.

diff --git i/builtin/rev-parse.c w/builtin/rev-parse.c
index c76b89d..98262b3 100644
--- i/builtin/rev-parse.c
+++ w/builtin/rev-parse.c
@@ -234,37 +234,35 @@ static int try_difference(const char *arg)
 	unsigned char sha1[20];
 	unsigned char end[20];
 	const char *next;
-	const char *this;
+	struct strbuf this = STRBUF_INIT;
 	int symmetric;
-	static const char head_by_default[] = "HEAD";
 
 	if (!(dotdot = strstr(arg, "..")))
 		return 0;
 	next = dotdot + 2;
-	this = arg;
 	symmetric = (*next == '.');
-
-	*dotdot = 0;
 	next += symmetric;
 
-	if (!*next)
-		next = head_by_default;
-	if (dotdot == arg)
-		this = head_by_default;
-
-	if (this == head_by_default && next == head_by_default &&
-	    !symmetric) {
+	if (!*next && dotdot == arg && !symmetric)
 		/*
 		 * Just ".."?  That is not a range but the
 		 * pathspec for the parent directory.
 		 */
-		*dotdot = '.';
 		return 0;
+
+	strbuf_add(&this, arg, dotdot - arg);
+
+	if (!*next)
+		next = "HEAD";
+	if (dotdot == arg) {
+		strbuf_reset(&this);
+		strbuf_addstr(&this, "HEAD");
 	}
 
-	if (!get_sha1_committish(this, sha1) && !get_sha1_committish(next, end)) {
+	if (!get_sha1_committish(this.buf, sha1)
+	    && !get_sha1_committish(next, end)) {
 		show_rev(NORMAL, end, next);
-		show_rev(symmetric ? NORMAL : REVERSED, sha1, this);
+		show_rev(symmetric ? NORMAL : REVERSED, sha1, this.buf);
 		if (symmetric) {
 			struct commit_list *exclude;
 			struct commit *a, *b;
@@ -279,9 +277,10 @@ static int try_difference(const char *arg)
 				exclude = n;
 			}
 		}
+		strbuf_release(&this);
 		return 1;
 	}
-	*dotdot = '.';
+	strbuf_release(&this);
 	return 0;
 }