From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 04 Nov 2026 23:06:49 GMT
Subject: Re: [PATCH 2/2] object-file: return error on index-pack abnormal exit
Message-ID: <xmqq8q38yz5i.fsf@gitster.g>
In-Reply-To: <20261103212410.1389988-3-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

> diff --git a/object-file.c b/object-file.c
> index a9800143bd..e04ce0bc11 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1429,8 +1429,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  	struct pack_header hdr;
>  	const char *hdr_err;
>  	int err_fd = opts->err_fd;
> -	int ret = -1;
> -	int status;
> +	int ret;
>  
>  	hdr_err = parse_pack_header(&hdr, pack_fd);
>  	if (hdr_err) {
> @@ -1462,12 +1461,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  		child.in = pack_fd;
>  		child.err = err_fd;
>  		child.git_cmd = 1;
> -		status = run_command(&child);
> -		if (status) {
> +		if (run_command(&child)) {
>  			strbuf_addstr(err_msg, "unpack-objects abnormal exit");
>  			return -1;
>  		}

OK.  A slight simplication that does not change any behaviour.

>  	} else {
> +		int report_err;
> +
>  		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
>  		push_header_arg(&child.args, &hdr);
>  
> @@ -1487,24 +1487,24 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  		child.in = pack_fd;
>  		child.err = err_fd;
>  		child.git_cmd = 1;
> -		status = start_command(&child);
> -		if (status) {
> +		if (start_command(&child)) {
>  			strbuf_addstr(err_msg, "index-pack fork failed");
>  			return -1;
>  		}
>  
> -		ret = read_index_pack_report(repo, child.out, &pack_name, NULL);
> +		report_err = read_index_pack_report(repo, child.out, &pack_name, NULL);
>  
>  		close(child.out);
>  
> -		status = finish_command(&child);
> -		if (status) {
> +		if (finish_command(&child)) {
>  			strbuf_addstr(err_msg, "index-pack abnormal exit");
> +			ret = -1;
>  			goto out;
>  		}

OK.  A slight simplication that does not change any behaviour again.

> -		if (ret) {
> +		if (report_err) {
>  			strbuf_addstr(err_msg, "index-pack failed to report pack name");
> +			ret = -1;
>  			goto out;
>  		}

I guess this also does not change a behaviour?  We used to relay the
error status returned by read_index_pack_report(), but now we force
the error code to an explicit -1.

Personally, I somehow find that the original "unless we positively
declare success, default to 'ret = -1', and if we may have detected
more specific error, return it back, and reset ret to 0 when we see
a success" easier to follow, but that may be subjective preference.

Will queue.  Thanks.