From: Derrick Stolee <stolee@gmail•com>
To: Taylor Blau <me@ttaylorr•com>, Junio C Hamano <gitster@pobox•com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails
Date: Sat, 30 Aug 2025 10:44:22 -0400 [thread overview]
Message-ID: <eac886a7-0f50-40fa-b10e-92c68d60bac7@gmail.com> (raw)
In-Reply-To: <aLECkfDqFVgNA1xh@nand.local>
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@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.
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
next prev parent reply other threads:[~2025-08-30 14:44 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-28 17:39 [PATCH 0/5] midx-write: fix segfault and do several cleanups Derrick Stolee via GitGitGadget
2025-08-28 17:39 ` [PATCH 1/5] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-08-28 20:19 ` Junio C Hamano
2025-08-29 1:20 ` Taylor Blau
2025-08-30 14:33 ` Derrick Stolee
2025-08-28 17:39 ` [PATCH 2/5] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-08-28 20:45 ` Junio C Hamano
2025-08-29 1:26 ` Taylor Blau
2025-08-28 17:39 ` [PATCH 3/5] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-08-28 20:51 ` Junio C Hamano
2025-08-29 1:29 ` Taylor Blau
2025-08-30 14:44 ` Derrick Stolee [this message]
2025-08-28 17:39 ` [PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-08-28 20:58 ` Junio C Hamano
2025-08-29 1:35 ` Taylor Blau
2025-08-28 17:39 ` [PATCH 5/5] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-08-28 21:01 ` Junio C Hamano
2025-08-29 1:35 ` Taylor Blau
2025-08-29 1:36 ` [PATCH 0/5] midx-write: fix segfault and do several cleanups Taylor Blau
2025-08-30 21:23 ` [PATCH v2 0/6] " Derrick Stolee via GitGitGadget
2025-08-30 21:23 ` [PATCH v2 1/6] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-09-03 10:14 ` Patrick Steinhardt
2025-09-05 18:58 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 2/6] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-05 19:03 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 3/6] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-08-30 21:23 ` [PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-05 19:05 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 5/6] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-08-30 21:23 ` [PATCH v2 6/6] midx-write: simplify error cases Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-03 18:43 ` Junio C Hamano
2025-09-05 19:26 ` [PATCH v3 0/6] midx-write: fix segfault and do several cleanups Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 1/6] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 2/6] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 3/6] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 4/6] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 5/6] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 6/6] midx-write: simplify error cases Derrick Stolee via GitGitGadget
2025-09-05 19:38 ` [PATCH v3 0/6] midx-write: fix segfault and do several cleanups Junio C Hamano
2025-09-05 19:57 ` Derrick Stolee
2025-09-11 23:13 ` Taylor Blau
2025-09-11 23:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eac886a7-0f50-40fa-b10e-92c68d60bac7@gmail.com \
--to=stolee@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox