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

[Junio C Hamano]

Jeff King  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 
> ---
>  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;
>   

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

[Jeff King]

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 
---
 revision.c | 177 +++--
 1 file changed, 102 insertions(+), 75 deletions(-)

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