[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 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 --]