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

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

On 11/10/26 22:13, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>>> 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.
> 
> Note that my comment you quoted was made with a misunderstanding
> of the patch and misses one important point, which is that
> read_index_pack_report() used to overwrite "ret" even when it
> succeeded, but the new code uses a separate variable to avoid
> clobbering "ret" when the call succeeds.
> 
> In other words, the original failed to honor the "have ret = -1
> and reset to 0 only after seeing everything go well" pattern,
> either, and Justin's patch is a definite improvement.

It is an improvement indeed. No arguments about it. I was just wondering 
if his change could also include a change like this on top:

diff --git a/object-file.c b/object-file.c
index e04ce0bc11..c52004014f 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1429,7 +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;
+	int ret = -1;

  	hdr_err = parse_pack_header(&hdr, pack_fd);
  	if (hdr_err) {
@@ -1498,13 +1498,11 @@ static int 
odb_transaction_files_write_pack(struct odb_transaction *base,

  		if (finish_command(&child)) {
  			strbuf_addstr(err_msg, "index-pack abnormal exit");
-			ret = -1;
  			goto out;
  		}

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


But, I would agree that this is a digression.

-- 
Sivaraam