Re: [PATCH 2/2] object-file: return error on index-pack abnormal exit

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On 11/5/26 04:36, Junio C Hamano wrote:
>> @@ -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.
>

+1. I too have a slight preference for the old flow of having ret = -1 
and resetting it when there's no error. It may not be worth a re-roll, 
though.

Otherwise, both patches look good to me.

-- 
Sivaraam