diff mbox series

[v2,13/13] name-rev: cleanup name_ref()

Message ID 20191112103821.30265-14-szeder.dev@gmail.com
State New, archived
Headers show
Series name-rev: eliminate recursion | expand

Commit Message

SZEDER Gábor Nov. 12, 2019, 10:38 a.m. UTC
Earlier patches in this series moved a couple of conditions from the
recursive name_rev() function into its caller name_ref(), for no other
reason than to make eliminating the recursion a bit easier to follow.

Since the previous patch name_rev() is not recursive anymore, so let's
move all those conditions back into name_rev().

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/name-rev.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Jonathan Tan Nov. 27, 2019, 6:01 p.m. UTC | #1
> Earlier patches in this series moved a couple of conditions from the
> recursive name_rev() function into its caller name_ref(), for no other
> reason than to make eliminating the recursion a bit easier to follow.
> 
> Since the previous patch name_rev() is not recursive anymore, so let's
> move all those conditions back into name_rev().

I don't really see the need for this code movement, to be honest. There
is no big difference in doing the checks in one place or the other, and
if you ask me, it might even be better to do it in the caller of
name_rev(), and leave name_rev() to handle only the naming.
SZEDER Gábor Dec. 9, 2019, 12:32 p.m. UTC | #2
On Wed, Nov 27, 2019 at 10:01:02AM -0800, Jonathan Tan wrote:
> > Earlier patches in this series moved a couple of conditions from the
> > recursive name_rev() function into its caller name_ref(), for no other
> > reason than to make eliminating the recursion a bit easier to follow.
> > 
> > Since the previous patch name_rev() is not recursive anymore, so let's
> > move all those conditions back into name_rev().
> 
> I don't really see the need for this code movement, to be honest. There
> is no big difference in doing the checks in one place or the other, and
> if you ask me, it might even be better to do it in the caller of
> name_rev(), and leave name_rev() to handle only the naming.

I think it does make sense: in my view the cutoff handling and calling
create_or_update_name() is part of "handling the naming", and it's
better when they are constrained to only a single function.
diff mbox series

Patch

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a3b796eac4..cc488ee319 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -107,12 +107,26 @@  static struct rev_name *create_or_update_name(struct commit *commit,
 
 static void name_rev(struct commit *start_commit,
 		const char *tip_name, timestamp_t taggerdate,
-		int from_tag)
+		int from_tag, int deref)
 {
 	struct prio_queue queue;
 	struct commit *commit;
 	struct commit **parents_to_queue = NULL;
 	size_t parents_to_queue_nr, parents_to_queue_alloc = 0;
+	char *to_free = NULL;
+
+	parse_commit(start_commit);
+	if (start_commit->date < cutoff)
+		return;
+
+	if (deref)
+		tip_name = to_free = xstrfmt("%s^0", tip_name);
+
+	if (!create_or_update_name(start_commit, tip_name, taggerdate, 0, 0,
+				   from_tag)) {
+		free(to_free);
+		return;
+	}
 
 	memset(&queue, 0, sizeof(queue)); /* Use the prio_queue as LIFO */
 	prio_queue_put(&queue, start_commit);
@@ -309,20 +323,7 @@  static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 		if (taggerdate == TIME_MAX)
 			taggerdate = commit->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		if (commit->date >= cutoff) {
-			const char *tip_name;
-			char *to_free = NULL;
-			if (deref)
-				tip_name = to_free = xstrfmt("%s^0", path);
-			else
-				tip_name = xstrdup(path);
-			if (create_or_update_name(commit, tip_name, taggerdate,
-						  0, 0, from_tag))
-				name_rev(commit, tip_name, taggerdate,
-					 from_tag);
-			else
-				free(to_free);
-		}
+		name_rev(commit, xstrdup(path), taggerdate, from_tag, deref);
 	}
 	return 0;
 }