# [PATCH 0/2] object-file: small error handling follow-ups 6 messages from 2026-11-03 to 2026-11-04. Participants: Justin Tobler, Karthik Nayak, Junio C Hamano. Thread: https://gitlist.dev/t/66760 ## Justin Tobler, 2026-11-03 21:24 Subject: [PATCH 0/2] object-file: small error handling follow-ups Message-ID: <20261103212410.1389988-1-jltobler@gmail.com> URL: https://gitlist.dev/e/20261103212410.1389988-1-jltobler%40gmail.com ``` 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 ``` ## Justin Tobler, 2026-11-03 21:24 Subject: [PATCH 1/2] object-file: warn on failed unlink Message-ID: <20261103212410.1389988-2-jltobler@gmail.com> URL: https://gitlist.dev/e/20261103212410.1389988-2-jltobler%40gmail.com In-Reply-To: <20261103212410.1389988-1-jltobler@gmail.com> ``` 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 --- 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 ``` ## Justin Tobler, 2026-11-03 21:24 Subject: [PATCH 2/2] object-file: return error on index-pack abnormal exit Message-ID: <20261103212410.1389988-3-jltobler@gmail.com> URL: https://gitlist.dev/e/20261103212410.1389988-3-jltobler%40gmail.com In-Reply-To: <20261103212410.1389988-1-jltobler@gmail.com> ``` 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 --- 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 ``` ## Karthik Nayak, 2026-11-04 13:37 Subject: Re: [PATCH 0/2] object-file: small error handling follow-ups Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRvbcjRt1MNn1ssun_Sfxm4ihQ%2Bprw3sxc7%3DV5FcpVkMQ%40mail.gmail.com In-Reply-To: <20261103212410.1389988-1-jltobler@gmail.com> ``` Justin Tobler 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! ``` ## Junio C Hamano, 2026-11-04 23:00 Subject: Re: [PATCH 1/2] object-file: warn on failed unlink Message-ID: URL: https://gitlist.dev/e/xmqqfqxgyzfo.fsf%40gitster.g In-Reply-To: <20261103212410.1389988-2-jltobler@gmail.com> ``` Justin Tobler 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 > --- > 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. ``` ## Junio C Hamano, 2026-11-04 23:06 Subject: Re: [PATCH 2/2] object-file: return error on index-pack abnormal exit Message-ID: URL: https://gitlist.dev/e/xmqq8q38yz5i.fsf%40gitster.g In-Reply-To: <20261103212410.1389988-3-jltobler@gmail.com> ``` Justin Tobler 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. ```