On 8/28/25 9:29 PM, Taylor Blau wrote:
On Thu, Aug 28, 2025 at 01:51:18PM -0700, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Derrick Stolee <stolee@xxxxxxxxx>
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@xxxxxxxxx>
---
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.
I'll look into adding this as a cleanup. This specific patch is
about adding the 'goto's where they were missing. I can make a new
patch that unifies the result initialization to -1 (and thus making
the method unified in returning -1 on error). There are many more
"result = 1" lines that need to change.
Thanks,
-Stolee
