name-rev: rewrite create_or_update_name()
diff mbox series

Message ID 20190922081846.14452-1-martin.agren@gmail.com
State New
Headers show
Series
  • name-rev: rewrite create_or_update_name()
Related show

Commit Message

Martin Ågren Sept. 22, 2019, 8:18 a.m. UTC
This code was moved straight out of name_rev(). As such, we inherited
the "goto" to jump from an if into an else-if. We also inherited the
fact that "nothing to do -- return NULL" is handled last.

Rewrite the function to first handle the "nothing to do" case. Then we
can handle the conditional allocation early before going on to populate
the struct. No need for goto-ing.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Hi SZEDER,

 For the record, --color-moved confirms that your patch is a move and
 the conversion around it looks good to me. I was a bit puzzled by what
 the moved code actually wanted to *do* and came up with this rewrite.

 I guess it's subjective which of these ways of writing this function is
 "better", but I found this rewrite helped in understanding this
 function. Feel free to pick up in a reroll, squash or ignore as you see
 fit. This is based on top of your whole series (your e5d77042f), but
 could perhaps go immediately after your patch 07/15.

 It seems there was some discussion around leaks and leak-plugs. That
 would conflict/interact with this. Instead of placing a call to free()
 just before the label so we can more or less goto around it, the middle
 section of this rewritten function would turn from "if no name,
 allocate one" to "if we have a name, free stuff, else allocate one".
 Again, it's subjective which way is "better", so trust your judgment.

 Martin

 builtin/name-rev.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Patch
diff mbox series

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 6969af76c4..03a5f0b189 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -83,21 +83,21 @@  static struct rev_name *create_or_update_name(struct commit *commit,
 {
 	struct rev_name *name = get_commit_rev_name(commit);
 
+	if (name && !is_better_name(name, taggerdate, distance, from_tag))
+		return NULL;
+
 	if (name == NULL) {
 		name = xmalloc(sizeof(*name));
 		set_commit_rev_name(commit, name);
-		goto copy_data;
-	} else if (is_better_name(name, taggerdate, distance, from_tag)) {
-copy_data:
-		name->tip_name = tip_name;
-		name->taggerdate = taggerdate;
-		name->generation = generation;
-		name->distance = distance;
-		name->from_tag = from_tag;
-
-		return name;
-	} else
-		return NULL;
+	}
+
+	name->tip_name = tip_name;
+	name->taggerdate = taggerdate;
+	name->generation = generation;
+	name->distance = distance;
+	name->from_tag = from_tag;
+
+	return name;
 }
 
 static void name_rev(struct commit *start_commit,