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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 9, 2022, 21:58 UTC
Message-ID: <Yiki7+BSi5sNamRf@nand.local>
In-Reply-To: <50873925-a5bc-7df8-1407-371eb904e38e@github.com>

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

Show 11 quoted lines

> On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:
>
> > +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