[PATCH 0/2] object-file: small error handling follow-ups

[PATCH 0/2] object-file: small error handling follow-ups

[Justin Tobler]

Greetings,

This small series is a follow-up to jt/odb-keep-lockfile-fix to address
some feedback that was submitted after the series was merged to next.
Both patches here make some small improvements to some error handling code.

Thanks,
-Justin

Justin Tobler (2):
  object-file: warn on failed unlink
  object-file: return error on index-pack abnormal exit

 object-file.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)


base-commit: 750f5de7dfe9a4062f95088212b063f189d50e91
-- 
2.56.0.173.gf57c9a7388

[PATCH 1/2] object-file: warn on failed unlink

[Justin Tobler]

When the "files" ODB transaction cleans up a temporary packfile or
".keep" lockfile on an error path, any failure to unlink the file is
silently ignored. A ".keep" file left behind this way prevents its
packfile from ever being removed by housekeeping.

Use `unlink_or_warn()` instead so that such failures are reported.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/object-file.c b/object-file.c
index 132aaab6f8..a9800143bd 100644
--- a/object-file.c
+++ b/object-file.c
@@ -823,7 +823,7 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
 	if (state->nr_written == 0) {
 		close(state->f->fd);
 		free_hashfile(state->f);
-		unlink(state->pack_tmp_name);
+		unlink_or_warn(state->pack_tmp_name);
 		goto clear_exit;
 	} else if (state->nr_written == 1) {
 		finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,
@@ -1276,13 +1276,13 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
 	if (write_in_full(fd, contents.buf, contents.len) < 0) {
 		ret = error_errno(_("unable to write '%s'"), lock_path.buf);
 		close(fd);
-		unlink(lock_path.buf);
+		unlink_or_warn(lock_path.buf);
 		goto out;
 	}
 
 	if (close(fd) < 0) {
 		ret = error_errno(_("unable to close '%s'"), lock_path.buf);
-		unlink(lock_path.buf);
+		unlink_or_warn(lock_path.buf);
 		goto out;
 	}
 
-- 
2.56.0.173.gf57c9a7388

Re: [PATCH 1/2] object-file: warn on failed unlink

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

> When the "files" ODB transaction cleans up a temporary packfile or
> ".keep" lockfile on an error path, any failure to unlink the file is
> silently ignored. A ".keep" file left behind this way prevents its
> packfile from ever being removed by housekeeping.
>
> Use `unlink_or_warn()` instead so that such failures are reported.
>
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  object-file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 132aaab6f8..a9800143bd 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -823,7 +823,7 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
>  	if (state->nr_written == 0) {
>  		close(state->f->fd);
>  		free_hashfile(state->f);
> -		unlink(state->pack_tmp_name);
> +		unlink_or_warn(state->pack_tmp_name);
>  		goto clear_exit;
>  	} else if (state->nr_written == 1) {
>  		finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,
> @@ -1276,13 +1276,13 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
>  	if (write_in_full(fd, contents.buf, contents.len) < 0) {
>  		ret = error_errno(_("unable to write '%s'"), lock_path.buf);
>  		close(fd);
> -		unlink(lock_path.buf);
> +		unlink_or_warn(lock_path.buf);
>  		goto out;
>  	}
>  
>  	if (close(fd) < 0) {
>  		ret = error_errno(_("unable to close '%s'"), lock_path.buf);
> -		unlink(lock_path.buf);
> +		unlink_or_warn(lock_path.buf);
>  		goto out;
>  	}

Trivially correct.

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

[Justin Tobler]

When git-index-pack(1) exits abnormally after reporting the packfile
name, `odb_transaction_files_write_pack()` records an error message in
`err_msg` but still returns success. Current callers ultimately discard
the return value, so this does not cause any issues at present, but the
return value should still reflect the failure.

Update `odb_transaction_files_write_pack()` to return an error value in
this case accordingly. While at it, simplify how the child process exit
statuses are checked.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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;
 		}
 	} 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;
 		}
 
-		if (ret) {
+		if (report_err) {
 			strbuf_addstr(err_msg, "index-pack failed to report pack name");
+			ret = -1;
 			goto out;
 		}
 
-- 
2.56.0.173.gf57c9a7388

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

[Junio C Hamano]

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.

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

[Justin Tobler]

On 26/11/04 03:06PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> > -		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.

That is correct, there is no behavior change here. The main change is
that `finish_command()` properly reports its error and is not
overwritten by `read_index_pack_report()`.

[snip]
> 
> Will queue.  Thanks.

Thanks for the review,
-Justin

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

[Kaartic Sivaraam]

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

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

[Junio C Hamano]

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.

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

[Kaartic Sivaraam]

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

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

[Justin Tobler]

On 26/11/10 11:27PM, Kaartic Sivaraam wrote:
> 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.

I think this is mostly just a matter of style preference. I'm inclined
to leave it as-is, but an open to change it anyone feels strongly.

-Justin

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

[Patrick Steinhardt]

On Tue, Nov 03, 2026 at 03:24:10PM -0600, Justin Tobler wrote:
> When git-index-pack(1) exits abnormally after reporting the packfile
> name, `odb_transaction_files_write_pack()` records an error message in
> `err_msg` but still returns success. Current callers ultimately discard
> the return value, so this does not cause any issues at present, but the
> return value should still reflect the failure.

Is this a theoretical concern or does git-index-pack(1) perform any
important steps after reporting the packfile name? I think we should of
course respect errors in either case, but I wonder whether this is a bug
fix or whether it's a simple cleanup.

Patrick

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

[Justin Tobler]

On 26/11/05 04:30PM, Patrick Steinhardt wrote:
> On Tue, Nov 03, 2026 at 03:24:10PM -0600, Justin Tobler wrote:
> > When git-index-pack(1) exits abnormally after reporting the packfile
> > name, `odb_transaction_files_write_pack()` records an error message in
> > `err_msg` but still returns success. Current callers ultimately discard
> > the return value, so this does not cause any issues at present, but the
> > return value should still reflect the failure.
> 
> Is this a theoretical concern or does git-index-pack(1) perform any
> important steps after reporting the packfile name? I think we should of
> course respect errors in either case, but I wonder whether this is a bug
> fix or whether it's a simple cleanup.

This is a theoretical concern. If `read_index_pack_report()` returned
success and finish_command() returned an error,
`odb_transaction_files_write_pack()` would append an error message but
return 0. This isn't really the correct behavior. In this case though,
it doesn't cause any bugs because the return value is ultimately
discarded by its callers.

-Justin

Re: [PATCH 0/2] object-file: small error handling follow-ups

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> Greetings,
>
> This small series is a follow-up to jt/odb-keep-lockfile-fix to address
> some feedback that was submitted after the series was merged to next.
> Both patches here make some small improvements to some error handling code.
>
> Thanks,
> -Justin
>
> Justin Tobler (2):
>   object-file: warn on failed unlink
>   object-file: return error on index-pack abnormal exit
>
>  object-file.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
>
> base-commit: 750f5de7dfe9a4062f95088212b063f189d50e91
> --
> 2.56.0.173.gf57c9a7388

So these seem to be the fixes for the comments I raised on your other
series. The patches look good. Thanks for following up!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]