Re: [PATCH v2 6/6] midx-write: simplify error cases

[Patrick Steinhardt <ps@pks.im>]

On Sat, Aug 30, 2025 at 09:23:27PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> 
> The write_midx_internal() method uses gotos to jump to a cleanup section to
> clear memory before returning 'result'. Since these jumps are more common
> for error conditions, initialize 'result' to -1 and then only set it to 0
> before returning with success. There are a couple places where we return
> with success via a jump.
> 
> This has the added benefit that the method now returns -1 on error instead
> of an inconsistent 1 or -1.
> 
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  midx-write.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/midx-write.c b/midx-write.c
> index 14a0947c46..047ffdcdbf 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1046,7 +1046,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
>  	int bitmapped_packs_concat_len = 0;
>  	int pack_name_concat_len = 0;
>  	int dropped_packs = 0;
> -	int result = 0;
> +	int result = -1;
>  	const char **keep_hashes = NULL;
>  	struct chunkfile *cf;

I personally prefer to keep the result uninitialized and then assign the
result of `error()` to it. It's almost the same lines of code as we have
right now, but it has the advantage that the compiler will complain
about `result` being uninitialized if we ever forget to set it. So it's
overall way more explicit, and the compiler protects us.

But seeing that Junio previously recommended to go into the direction of
setting it to `-1` I won't insist on such a refactoring. So please feel
free to ignore this comment.

Patrick