From: Derrick Stolee Date: Sat, 30 Aug 2025 14:44:22 GMT Subject: Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails Message-ID: In-Reply-To: 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" writes: >> >>> From: Derrick Stolee >>> >>> 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 >>> --- >>> 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