| Message ID | 20191112103821.30265-14-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: eliminate recursion | expand |
> 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.
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 --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; }
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(-)