Re: [PATCH 10/24] revisions API users: use release_revisions() in builtin/log.c

On Wed, Mar 09, 2022 at 02:12:15PM -0500, Derrick Stolee wrote:

On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

+static int cmd_log_deinit(int ret, struct rev_info *rev)
+{
+	release_revisions(rev);
+	return ret;
+}

This pattern of passing a return value through the helper
function is a clever way to get around adding "int ret = ...;
release(); return ret;" lines.

Clever indeed. I originally thought that cmd_log_deinit() was going to
be a wrapper around release_revisions() that also freed any log-specific
bits of the rev_info structure that are used exclusively by this file.

But that is not a great pattern, since having to track which file(s) use
which field(s) of the rev_info structure sounds cumbersome, error-prone,
and fragile. So I'm glad to see that it's really just used to
temporarily hold a return value.

I wouldn't be sad to see this as a macro, either, maybe something like:

    #define LOG_TEARDOWN(ret, rev) \
        do { release_revisions(rev); return ret; } while (0);

to make it clear(er) that this has a pretty narrow single purpose. But
even that doesn't seem like a great idea, since many of the uses have
the result of a function call as the first argument, which obviously
would not work with this macro.

So it's probably fine as-is ;).

Thanks,
Taylor