git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: [PATCH] name-rev: rewrite create_or_update_name()
Date: Sun, 22 Sep 2019 10:18:46 +0200	[thread overview]
Message-ID: <20190922081846.14452-1-martin.agren@gmail.com> (raw)
In-Reply-To: <20190919214712.7348-8-szeder.dev@gmail.com>

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(-)

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,
-- 
2.23.0


  parent reply	other threads:[~2019-09-22  8:19 UTC|newest]

Thread overview: 98+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-19 21:46 [PATCH 00/15] name-rev: eliminate recursion SZEDER Gábor
2019-09-19 21:46 ` [PATCH 01/15] t6120-describe: correct test repo history graph in comment SZEDER Gábor
2019-09-20 21:47   ` Junio C Hamano
2019-09-20 22:29     ` SZEDER Gábor
2019-09-28  4:06       ` Junio C Hamano
2019-09-19 21:46 ` [PATCH 02/15] t6120-describe: modernize the 'check_describe' helper SZEDER Gábor
2019-09-20 21:49   ` Junio C Hamano
2019-09-19 21:46 ` [PATCH 03/15] name-rev: use strip_suffix() in get_rev_name() SZEDER Gábor
2019-09-20 16:36   ` René Scharfe
2019-09-20 17:10     ` SZEDER Gábor
2019-09-19 21:46 ` [PATCH 04/15] name-rev: avoid unnecessary cast in name_ref() SZEDER Gábor
2019-09-20 16:37   ` René Scharfe
2019-09-19 21:47 ` [PATCH 05/15] name-rev: use sizeof(*ptr) instead of sizeof(type) in allocation SZEDER Gábor
2019-09-20 15:11   ` Derrick Stolee
2019-09-20 15:40     ` SZEDER Gábor
2019-09-20 16:37   ` René Scharfe
2019-09-19 21:47 ` [PATCH 06/15] t6120: add a test to cover inner conditions in 'git name-rev's name_rev() SZEDER Gábor
2019-09-20 15:14   ` Derrick Stolee
2019-09-20 15:44     ` SZEDER Gábor
2019-09-19 21:47 ` [PATCH 07/15] name-rev: extract creating/updating a 'struct name_rev' into a helper SZEDER Gábor
2019-09-20 15:18   ` Derrick Stolee
2019-09-22  8:18   ` Martin Ågren [this message]
2019-12-09 12:43     ` [PATCH] name-rev: rewrite create_or_update_name() SZEDER Gábor
2019-09-19 21:47 ` [PATCH 08/15] name-rev: pull out deref handling from the recursion SZEDER Gábor
2019-09-20 15:21   ` Derrick Stolee
2019-09-20 17:42     ` SZEDER Gábor
2019-09-20 16:37   ` René Scharfe
2019-09-20 18:13     ` SZEDER Gábor
2019-09-20 18:14       ` SZEDER Gábor
2019-09-21  9:57         ` SZEDER Gábor
2019-09-21 12:37           ` René Scharfe
2019-09-22 19:05             ` SZEDER Gábor
2019-09-23 18:43               ` René Scharfe
2019-09-23 18:59                 ` SZEDER Gábor
2019-09-23 19:55                   ` René Scharfe
2019-09-23 20:47                     ` SZEDER Gábor
2019-09-24 17:03                       ` René Scharfe
2019-09-26 17:33                         ` SZEDER Gábor
2019-09-21 12:37       ` René Scharfe
2019-09-21 14:21         ` SZEDER Gábor
2019-09-21 15:52           ` René Scharfe
2019-09-19 21:47 ` [PATCH 09/15] name-rev: restructure parsing commits and applying date cutoff SZEDER Gábor
2019-09-21 12:37   ` René Scharfe
2019-09-19 21:47 ` [PATCH 10/15] name-rev: restructure creating/updating 'struct rev_name' instances SZEDER Gábor
2019-09-20 15:27   ` Derrick Stolee
2019-09-20 17:09     ` SZEDER Gábor
2019-09-19 21:47 ` [PATCH 11/15] name-rev: drop name_rev()'s 'generation' and 'distance' parameters SZEDER Gábor
2019-09-19 21:47 ` [PATCH 12/15] name-rev: eliminate recursion in name_rev() SZEDER Gábor
2019-09-19 21:47 ` [PATCH 13/15] name-rev: cleanup name_ref() SZEDER Gábor
2019-09-19 21:47 ` [PATCH 14/15] name-rev: plug a memory leak in name_rev() SZEDER Gábor
2019-09-19 21:47 ` [PATCH 14/15] name-rev: plug memory leak in name_rev() in the deref case SZEDER Gábor
2019-09-19 22:47   ` SZEDER Gábor
2019-09-19 21:47 ` [PATCH 15/15] name-rev: plug a " SZEDER Gábor
2019-09-20 15:35   ` Derrick Stolee
2019-09-19 21:47 ` [PATCH 15/15] name-rev: plug memory leak in name_rev() SZEDER Gábor
2019-09-19 22:48   ` SZEDER Gábor
2019-09-20 15:37 ` [PATCH 00/15] name-rev: eliminate recursion Derrick Stolee
2019-09-20 17:37   ` SZEDER Gábor
2019-11-12 10:38 ` [PATCH v2 00/13] " SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 01/13] t6120-describe: correct test repo history graph in comment SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 02/13] t6120-describe: modernize the 'check_describe' helper SZEDER Gábor
2019-11-27 18:02     ` Jonathan Tan
2019-11-12 10:38   ` [PATCH v2 03/13] name-rev: use strbuf_strip_suffix() in get_rev_name() SZEDER Gábor
2019-11-12 19:02     ` René Scharfe
2019-11-12 10:38   ` [PATCH v2 04/13] name-rev: avoid unnecessary cast in name_ref() SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 05/13] name-rev: use sizeof(*ptr) instead of sizeof(type) in allocation SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 06/13] t6120: add a test to cover inner conditions in 'git name-rev's name_rev() SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 07/13] name-rev: extract creating/updating a 'struct name_rev' into a helper SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 08/13] name-rev: pull out deref handling from the recursion SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 09/13] name-rev: restructure parsing commits and applying date cutoff SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 10/13] name-rev: restructure creating/updating 'struct rev_name' instances SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 11/13] name-rev: drop name_rev()'s 'generation' and 'distance' parameters SZEDER Gábor
2019-11-27 18:13     ` Jonathan Tan
2019-11-12 10:38   ` [PATCH v2 12/13] name-rev: eliminate recursion in name_rev() SZEDER Gábor
2019-11-27 17:57     ` Jonathan Tan
2019-12-09 12:22       ` SZEDER Gábor
2019-11-12 10:38   ` [PATCH v2 13/13] name-rev: cleanup name_ref() SZEDER Gábor
2019-11-27 18:01     ` Jonathan Tan
2019-12-09 12:32       ` SZEDER Gábor
2019-11-12 19:17   ` [PATCH v2 00/13] name-rev: eliminate recursion Johannes Schindelin
2019-11-13 19:25     ` Sebastiaan Dammann
2019-12-09 11:52   ` [PATCH v3 00/14] " SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 01/14] t6120-describe: correct test repo history graph in comment SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 02/14] t6120-describe: modernize the 'check_describe' helper SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 03/14] name-rev: use strbuf_strip_suffix() in get_rev_name() SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 04/14] name-rev: avoid unnecessary cast in name_ref() SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 05/14] name-rev: use sizeof(*ptr) instead of sizeof(type) in allocation SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 06/14] t6120: add a test to cover inner conditions in 'git name-rev's name_rev() SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 07/14] name-rev: extract creating/updating a 'struct name_rev' into a helper SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 08/14] name-rev: pull out deref handling from the recursion SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 09/14] name-rev: restructure parsing commits and applying date cutoff SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 10/14] name-rev: restructure creating/updating 'struct rev_name' instances SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 11/14] name-rev: drop name_rev()'s 'generation' and 'distance' parameters SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 12/14] name-rev: use 'name->tip_name' instead of 'tip_name' SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 13/14] name-rev: eliminate recursion in name_rev() SZEDER Gábor
2019-12-09 11:52     ` [PATCH v3 14/14] name-rev: cleanup name_ref() SZEDER Gábor
2019-12-09 15:08     ` [PATCH v3 00/14] name-rev: eliminate recursion Derrick Stolee
2019-12-11 17:33       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190922081846.14452-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).