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

[Junio C Hamano <gitster@pobox.com>]

"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.

> diff --git a/midx-write.c b/midx-write.c
> index 85b2d471ef..f2d9a990e6 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1321,13 +1321,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
>  		incr = mks_tempfile_m(midx_name.buf, 0444);
>  		if (!incr) {
>  			error(_("unable to create temporary MIDX layer"));
> -			return -1;
> +			result = -1;
> +			goto cleanup;
>  		}
>  
>  		if (adjust_shared_perm(r, get_tempfile_path(incr))) {
>  			error(_("unable to adjust shared permissions for '%s'"),
>  			      get_tempfile_path(incr));
> -			return -1;
> +			result = -1;
> +			goto cleanup;
>  		}
>  
>  		f = hashfd(r->hash_algo, get_tempfile_fd(incr),
> @@ -1427,18 +1429,22 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
>  
>  		if (!chainf) {
>  			error_errno(_("unable to open multi-pack-index chain file"));
> -			return -1;
> +			result = -1;
> +			goto cleanup;
>  		}
>  
> -		if (link_midx_to_chain(ctx.base_midx) < 0)
> -			return -1;
> +		if (link_midx_to_chain(ctx.base_midx) < 0) {
> +			result = -1;
> +			goto cleanup;
> +		}
>  
>  		get_split_midx_filename_ext(r->hash_algo, &final_midx_name,
>  					    object_dir, midx_hash, MIDX_EXT_MIDX);
>  
>  		if (rename_tempfile(&incr, final_midx_name.buf) < 0) {
>  			error_errno(_("unable to rename new multi-pack-index layer"));
> -			return -1;
> +			result = -1;
> +			goto cleanup;
>  		}
>  
>  		strbuf_release(&final_midx_name);