Re: [PATCH 05/15] handle_revision_arg: add handle_dotdot() helper

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

Jeff King <peff@peff.net> writes:

> The handle_revision_arg function is rather long, and a big
> chunk of it is handling the range operators. Let's pull that
> out to a separate helper. While we're doing so, we can clean
> up a few of the rough edges that made the flow hard to
> follow:
>
>   - instead of manually restoring *dotdot (that we overwrote
>     with a NUL), do the real work in a sub-helper, which
>     makes it clear that the munge/restore lines are a
>     matched pair
>
>   - eliminate a goto which wasn't actually used for control
>     flow, but only to avoid duplicating a few lines
>     (instead, those lines are pushed into another helper
>     function)
>
>   - use early returns instead of deep nesting
>
>   - consistently name all variables for the left-hand side
>     of the range as "a" (rather than "this" or "from") and
>     the right-hand side as "b" (rather than "next", or using
>     the unadorned "sha1" or "flags" from the main function).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 177 +++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 102 insertions(+), 75 deletions(-)

Nicely done.  Together with [PATCH 04/15] the resulting code wrt ".."
is much easier to follow.

> diff --git a/revision.c b/revision.c
> index dec06ff8b..eb45501fd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1429,10 +1429,109 @@ static void prepare_show_merge(struct rev_info *revs)
>  	revs->limited = 1;
>  }
>  
> +static int dotdot_missing(const char *arg, char *dotdot,
> +			  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);
> +}
> +
> +static int handle_dotdot_1(const char *arg, char *dotdot,
> +			   struct rev_info *revs, int flags,
> +			   int cant_be_filename)
> +{
> +	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);
> +
> +	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";
> +
> +	if (get_sha1_committish(a_name, a_oid.hash) ||
> +	    get_sha1_committish(b_name, b_oid.hash))
> +		return -1;
> +
> +	if (!cant_be_filename) {
> +		*dotdot = '.';
> +		verify_non_filename(revs->prefix, arg);
> +		*dotdot = '\0';
> +	}
> +
> +	a_obj = parse_object(a_oid.hash);
> +	b_obj = parse_object(b_oid.hash);
> +	if (!a_obj || !b_obj)
> +		return dotdot_missing(arg, dotdot, revs, symmetric);
> +
> +	if (!symmetric) {
> +		/* just A..B */
> +		b_flags = flags;
> +		a_flags = flags_exclude;
> +	} else {
> +		/* A...B -- find merge bases between the two */
> +		struct commit *a, *b;
> +		struct commit_list *exclude;
> +
> +		a = lookup_commit_reference(a_obj->oid.hash);
> +		b = lookup_commit_reference(b_obj->oid.hash);
> +		if (!a || !b)
> +			return dotdot_missing(arg, dotdot, revs, symmetric);
> +
> +		exclude = get_merge_bases(a, b);
> +		add_rev_cmdline_list(revs, exclude, REV_CMD_MERGE_BASE,
> +				     flags_exclude);
> +		add_pending_commit_list(revs, exclude, flags_exclude);
> +		free_commit_list(exclude);
> +
> +		b_flags = flags;
> +		a_flags = flags | SYMMETRIC_LEFT;
> +	}
> +
> +	a_obj->flags |= a_flags;
> +	b_obj->flags |= b_flags;
> +	add_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags);
> +	add_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags);
> +	add_pending_object(revs, a_obj, a_name);
> +	add_pending_object(revs, b_obj, b_name);
> +	return 0;
> +}
> +
> +static int handle_dotdot(const char *arg,
> +			 struct rev_info *revs, int flags,
> +			 int cant_be_filename)
> +{
> +	char *dotdot = strstr(arg, "..");
> +	int ret;
> +
> +	if (!dotdot)
> +		return -1;
> +
> +	*dotdot = '\0';
> +	ret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename);
> +	*dotdot = '.';
> +
> +	return ret;
> +}
> +
>  int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
>  {
>  	struct object_context oc;
> -	char *dotdot;
>  	char *mark;
>  	struct object *object;
>  	unsigned char sha1[20];
> @@ -1451,80 +1550,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
>  		return -1;
>  	}
>  
> -	dotdot = strstr(arg, "..");
> -	if (dotdot) {
> -		unsigned char from_sha1[20];
> -		const char *next = dotdot + 2;
> -		const char *this = arg;
> -		int symmetric = *next == '.';
> -		unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
> -		unsigned int a_flags;
> -
> -		*dotdot = 0;
> -		next += symmetric;
> -
> -		if (!*next)
> -			next = "HEAD";
> -		if (dotdot == arg)
> -			this = "HEAD";
> -		if (!get_sha1_committish(this, from_sha1) &&
> -		    !get_sha1_committish(next, sha1)) {
> -			struct object *a_obj, *b_obj;
> -
> -			if (!cant_be_filename) {
> -				*dotdot = '.';
> -				verify_non_filename(revs->prefix, arg);
> -				*dotdot = '\0';
> -			}
> -
> -			a_obj = parse_object(from_sha1);
> -			b_obj = parse_object(sha1);
> -			if (!a_obj || !b_obj) {
> -			missing:
> -				*dotdot = '.';
> -				if (revs->ignore_missing)
> -					return 0;
> -				die(symmetric
> -				    ? "Invalid symmetric difference expression %s"
> -				    : "Invalid revision range %s", arg);
> -			}
> -
> -			if (!symmetric) {
> -				/* just A..B */
> -				a_flags = flags_exclude;
> -			} else {
> -				/* A...B -- find merge bases between the two */
> -				struct commit *a, *b;
> -				struct commit_list *exclude;
> -
> -				a = lookup_commit_reference(a_obj->oid.hash);
> -				b = lookup_commit_reference(b_obj->oid.hash);
> -				if (!a || !b)
> -					goto missing;
> -				exclude = get_merge_bases(a, b);
> -				add_rev_cmdline_list(revs, exclude,
> -						     REV_CMD_MERGE_BASE,
> -						     flags_exclude);
> -				add_pending_commit_list(revs, exclude,
> -							flags_exclude);
> -				free_commit_list(exclude);
> -
> -				a_flags = flags | SYMMETRIC_LEFT;
> -			}
> -
> -			a_obj->flags |= a_flags;
> -			b_obj->flags |= flags;
> -			add_rev_cmdline(revs, a_obj, this,
> -					REV_CMD_LEFT, a_flags);
> -			add_rev_cmdline(revs, b_obj, next,
> -					REV_CMD_RIGHT, flags);
> -			add_pending_object(revs, a_obj, this);
> -			add_pending_object(revs, b_obj, next);
> -			*dotdot = '.';
> -			return 0;
> -		}
> -		*dotdot = '.';
> -	}
> +	if (!handle_dotdot(arg, revs, flags, revarg_opt))
> +		return 0;
>  
>  	mark = strstr(arg, "^@");
>  	if (mark && !mark[2]) {