| Message ID | 20190922081846.14452-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: rewrite create_or_update_name() | expand |
On Sun, Sep 22, 2019 at 10:18:46AM +0200, Martin Ågren wrote: > 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. > 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. Yeah, I had a bit of a "Huh?!" moment myself looking at that goto jumping over the condition, too... Initially I left it as-is to keep this patch a pure code movement, and that others might have a bit of fun as well when they stumble upon it in the future ;) > It seems there was some discussion around leaks and leak-plugs. That > would conflict/interact with this. And I didn't pick it up in later versions, because René's plans to clean up memory ownership would deal with it (and with much more) as well.
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,
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(-)