Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 29, 2025, 01:29 UTC
Message-ID: <aLECkfDqFVgNA1xh@nand.local>
In-Reply-To: <xmqqwm6ntbjt.fsf@gitster.g>

On Thu, Aug 28, 2025 at 01:51:18PM -0700, Junio C Hamano wrote:

Show 37 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Derrick Stolee <stolee@gmail.com>
> >
> > The incremental mode of writing a multi-pack-index has a few extra
> > conditions that could lead to failure, but these are currently
> > short-ciruiting with 'return -1' instead of setting the method's
> > 'result' variable and going to the cleanup tag.
> >
> > Replace these returns with gotos to avoid memory issues when exiting
> > early due to error conditions.
> >
> > Unfortunately, these error conditions are difficult to reproduce with
> > test cases, which is perhaps one reason why the memory loss was not
> > caught by existing test cases in memory tracking modes.
> >
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> > ---
> >  midx-write.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
>
> Good thinking, but may I suggest us to go one more step to adopt
> even better convention if we were to do this?
>
> Pessimistically initialize the "result" to -1 and let many "goto
> cleanup" just jump there.  And have "result = 0" just before the
> cleanup label where the success code path joins the final cleanup
> part of the function.
>
> This is often the right way to make the flow easier to see, because
> often the success code path is straight forward, and these error
> conditions are what employ the "goto cleanup" from many places.  By
> starting pessimistic, and declaring the success at the very end of
> the straight-forward success case code path, all other flows to the
> clean-up labels do not have to set the "ah I failed" flag.  It would
> eliminate the need for patches like the previous step if the
> original were following that pattern.

Alternatively replacing something like:

    error(...);
    result = -1;
    goto cleanup;

with just

    result = error(...);
    goto cleanup;

would IMHO make the code easier to read, though I agree that nothing is forcing us to remember to assign result in the first place ;-). I am not sure the pessimistic initialization is better in all cases either, since we have to remember to place it before any "cleanup" label, and make sure that that does not regress.

So, I dunno. I'm OK with what is written here, and I think we could certainly have a separate discussion to perhaps have CodingGuidelines take a stronger stance here.

Thanks, Taylor