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

> On Thu, Aug 28, 2025 at 01:51:18PM -0700, Junio C Hamano wrote:
>> "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.