Re: [PATCH 1/4] revision: make handle_dotdot() interface less confusing

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> There are two very subtle bits to the way we parse ".." (and "...")
> range operators:
>
>  1. In handle_dotdot_1(), we assume that the incoming arguments "dotdot"
>     and "arg" are part of the same string, with the first digit of the

"digit" -> "dot".

>     range-operator blanked to a NUL. Then when we want the full name
>     (e.g., to report an error), we replace the NUL with a dot to restore
>     the original string.
>
>  2. In handle_dotdot(), we take in a const string, but then we modify it
>     by overwriting the range operator with a NUL. This has worked OK in
>     practice since we tend to pass in buffers that are actually
>     writeable (including argv), but segfaults with something like:
>
>       handle_revision_arg("..HEAD", &revs, 0, 0);
>
>     On top of that, building with recent versions of glibc causes the
>     compiler to complain, because it notices when we use strchr() or
>     strstr() to launder away constness (basically detecting the
>     possibility of the segfault above via the type system).
>
> Instead of munging the buffer, let's instead make a temporary copy of
> the left-hand side of the range operator. That avoids any const
> violations, and lets us pass around the parsed elements independently:
> the left-hand side, the right-hand side, the number of dots (via the
> "symmetric" flag), and the original full string for error messages.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

OK.  I was hoping if we can do without a temporary allocation, but
the const-string "..HEAD" example does make it clear that it is not
something we can achieve easily.

And once we accept that it is inevitable to make a copy, everything
else falls into the right place.

> diff --git a/revision.c b/revision.c
> index 31808e3df0..f61262436f 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2038,41 +2038,32 @@ static void prepare_show_merge(struct rev_info *revs)
>  	free(prune);
>  }
>  
> -static int dotdot_missing(const char *arg, char *dotdot,
> +static int dotdot_missing(const char *full_name,
>  			  struct rev_info *revs, int symmetric)
>  {
>  	if (revs->ignore_missing)
>  		return 0;
> -	/* de-munge so we report the full argument */
> -	*dotdot = '.';
>  	die(symmetric
>  	    ? "Invalid symmetric difference expression %s"
> -	    : "Invalid revision range %s", arg);
> +	    : "Invalid revision range %s", full_name);
>  }
>  
> -static int handle_dotdot_1(const char *arg, char *dotdot,
> +static int handle_dotdot_1(const char *a_name, const char *b_name,
> +			   const char *full_name, int symmetric,
>  			   struct rev_info *revs, int flags,
>  			   int cant_be_filename,
>  			   struct object_context *a_oc,
>  			   struct object_context *b_oc)
>  {
> -	const char *a_name, *b_name;
>  	struct object_id a_oid, b_oid;
>  	struct object *a_obj, *b_obj;
>  	unsigned int a_flags, b_flags;
> -	int symmetric = 0;
>  	unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
>  	unsigned int oc_flags = GET_OID_COMMITTISH | GET_OID_RECORD_PATH;
>  
> -	a_name = arg;
>  	if (!*a_name)
>  		a_name = "HEAD";
>  
> -	b_name = dotdot + 2;
> -	if (*b_name == '.') {
> -		symmetric = 1;
> -		b_name++;
> -	}
>  	if (!*b_name)
>  		b_name = "HEAD";
>  
> @@ -2081,15 +2072,13 @@ static int handle_dotdot_1(const char *arg, char *dotdot,
>  		return -1;
>  
>  	if (!cant_be_filename) {
> -		*dotdot = '.';
> -		verify_non_filename(revs->prefix, arg);
> -		*dotdot = '\0';
> +		verify_non_filename(revs->prefix, full_name);
>  	}
>  
>  	a_obj = parse_object(revs->repo, &a_oid);
>  	b_obj = parse_object(revs->repo, &b_oid);
>  	if (!a_obj || !b_obj)
> -		return dotdot_missing(arg, dotdot, revs, symmetric);
> +		return dotdot_missing(full_name, revs, symmetric);
>  
>  	if (!symmetric) {
>  		/* just A..B */
> @@ -2103,7 +2092,7 @@ static int handle_dotdot_1(const char *arg, char *dotdot,
>  		a = lookup_commit_reference(revs->repo, &a_obj->oid);
>  		b = lookup_commit_reference(revs->repo, &b_obj->oid);
>  		if (!a || !b)
> -			return dotdot_missing(arg, dotdot, revs, symmetric);
> +			return dotdot_missing(full_name, revs, symmetric);
>  
>  		if (repo_get_merge_bases(the_repository, a, b, &exclude) < 0) {
>  			commit_list_free(exclude);
> @@ -2132,16 +2121,23 @@ static int handle_dotdot(const char *arg,
>  			 int cant_be_filename)
>  {
>  	struct object_context a_oc = {0}, b_oc = {0};
> -	char *dotdot = strstr(arg, "..");
> +	const char *dotdot = strstr(arg, "..");
> +	char *tmp;
> +	int symmetric = 0;
>  	int ret;
>  
>  	if (!dotdot)
>  		return -1;
>  
> -	*dotdot = '\0';
> -	ret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename,
> -			      &a_oc, &b_oc);
> -	*dotdot = '.';
> +	tmp = xmemdupz(arg, dotdot - arg);
> +	dotdot += 2;
> +	if (*dotdot == '.') {
> +		symmetric = 1;
> +		dotdot++;
> +	}
> +	ret = handle_dotdot_1(tmp, dotdot, arg, symmetric, revs, flags,
> +			      cant_be_filename, &a_oc, &b_oc);
> +	free(tmp);
>  
>  	object_context_release(&a_oc);
>  	object_context_release(&b_oc);