{"thread":{"id":"66760","subject":"[PATCH 0/2] object-file: small error handling follow-ups","startedAt":"2026-11-03T21:24:08Z","lastAt":"2026-11-04T23:06:49Z","messageCount":6,"participants":["Justin Tobler","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"557375","messageId":"20261103212410.1389988-1-jltobler@gmail.com","threadId":"66760","inReplyTo":null,"subject":"[PATCH 0/2] object-file: small error handling follow-ups","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-11-03T21:24:08Z","receivedAt":"2026-11-03T21:24:08Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nThis small series is a follow-up to jt/odb-keep-lockfile-fix to address\nsome feedback that was submitted after the series was merged to next.\nBoth patches here make some small improvements to some error handling code.\n\nThanks,\n-Justin\n\nJustin Tobler (2):\n  object-file: warn on failed unlink\n  object-file: return error on index-pack abnormal exit\n\n object-file.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\n\nbase-commit: 750f5de7dfe9a4062f95088212b063f189d50e91\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"557376","messageId":"20261103212410.1389988-2-jltobler@gmail.com","threadId":"66760","inReplyTo":"20261103212410.1389988-1-jltobler@gmail.com","subject":"[PATCH 1/2] object-file: warn on failed unlink","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-11-03T21:24:09Z","receivedAt":"2026-11-03T21:24:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When the \"files\" ODB transaction cleans up a temporary packfile or\n\".keep\" lockfile on an error path, any failure to unlink the file is\nsilently ignored. A \".keep\" file left behind this way prevents its\npackfile from ever being removed by housekeeping.\n\nUse `unlink_or_warn()` instead so that such failures are reported.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n object-file.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 132aaab6f8..a9800143bd 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -823,7 +823,7 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction\n \tif (state->nr_written == 0) {\n \t\tclose(state->f->fd);\n \t\tfree_hashfile(state->f);\n-\t\tunlink(state->pack_tmp_name);\n+\t\tunlink_or_warn(state->pack_tmp_name);\n \t\tgoto clear_exit;\n \t} else if (state->nr_written == 1) {\n \t\tfinalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,\n@@ -1276,13 +1276,13 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,\n \tif (write_in_full(fd, contents.buf, contents.len) < 0) {\n \t\tret = error_errno(_(\"unable to write '%s'\"), lock_path.buf);\n \t\tclose(fd);\n-\t\tunlink(lock_path.buf);\n+\t\tunlink_or_warn(lock_path.buf);\n \t\tgoto out;\n \t}\n \n \tif (close(fd) < 0) {\n \t\tret = error_errno(_(\"unable to close '%s'\"), lock_path.buf);\n-\t\tunlink(lock_path.buf);\n+\t\tunlink_or_warn(lock_path.buf);\n \t\tgoto out;\n \t}\n \n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"557377","messageId":"20261103212410.1389988-3-jltobler@gmail.com","threadId":"66760","inReplyTo":"20261103212410.1389988-1-jltobler@gmail.com","subject":"[PATCH 2/2] object-file: return error on index-pack abnormal exit","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-11-03T21:24:10Z","receivedAt":"2026-11-03T21:24:10Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When git-index-pack(1) exits abnormally after reporting the packfile\nname, `odb_transaction_files_write_pack()` records an error message in\n`err_msg` but still returns success. Current callers ultimately discard\nthe return value, so this does not cause any issues at present, but the\nreturn value should still reflect the failure.\n\nUpdate `odb_transaction_files_write_pack()` to return an error value in\nthis case accordingly. While at it, simplify how the child process exit\nstatuses are checked.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n object-file.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex a9800143bd..e04ce0bc11 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1429,8 +1429,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n \tstruct pack_header hdr;\n \tconst char *hdr_err;\n \tint err_fd = opts->err_fd;\n-\tint ret = -1;\n-\tint status;\n+\tint ret;\n \n \thdr_err = parse_pack_header(&hdr, pack_fd);\n \tif (hdr_err) {\n@@ -1462,12 +1461,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n \t\tchild.in = pack_fd;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n-\t\tstatus = run_command(&child);\n-\t\tif (status) {\n+\t\tif (run_command(&child)) {\n \t\t\tstrbuf_addstr(err_msg, \"unpack-objects abnormal exit\");\n \t\t\treturn -1;\n \t\t}\n \t} else {\n+\t\tint report_err;\n+\n \t\tstrvec_pushl(&child.args, \"index-pack\", \"--stdin\", NULL);\n \t\tpush_header_arg(&child.args, &hdr);\n \n@@ -1487,24 +1487,24 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n \t\tchild.in = pack_fd;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n-\t\tstatus = start_command(&child);\n-\t\tif (status) {\n+\t\tif (start_command(&child)) {\n \t\t\tstrbuf_addstr(err_msg, \"index-pack fork failed\");\n \t\t\treturn -1;\n \t\t}\n \n-\t\tret = read_index_pack_report(repo, child.out, &pack_name, NULL);\n+\t\treport_err = read_index_pack_report(repo, child.out, &pack_name, NULL);\n \n \t\tclose(child.out);\n \n-\t\tstatus = finish_command(&child);\n-\t\tif (status) {\n+\t\tif (finish_command(&child)) {\n \t\t\tstrbuf_addstr(err_msg, \"index-pack abnormal exit\");\n+\t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n \n-\t\tif (ret) {\n+\t\tif (report_err) {\n \t\t\tstrbuf_addstr(err_msg, \"index-pack failed to report pack name\");\n+\t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n \n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"557438","messageId":"CAOLa=ZRvbcjRt1MNn1ssun_Sfxm4ihQ+prw3sxc7=V5FcpVkMQ@mail.gmail.com","threadId":"66760","inReplyTo":"20261103212410.1389988-1-jltobler@gmail.com","subject":"Re: [PATCH 0/2] object-file: small error handling follow-ups","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-11-04T13:37:38Z","receivedAt":"2026-11-04T13:37:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> Greetings,\n>\n> This small series is a follow-up to jt/odb-keep-lockfile-fix to address\n> some feedback that was submitted after the series was merged to next.\n> Both patches here make some small improvements to some error handling code.\n>\n> Thanks,\n> -Justin\n>\n> Justin Tobler (2):\n>   object-file: warn on failed unlink\n>   object-file: return error on index-pack abnormal exit\n>\n>  object-file.c | 26 +++++++++++++-------------\n>  1 file changed, 13 insertions(+), 13 deletions(-)\n>\n>\n> base-commit: 750f5de7dfe9a4062f95088212b063f189d50e91\n> --\n> 2.56.0.173.gf57c9a7388\n\nSo these seem to be the fixes for the comments I raised on your other\nseries. The patches look good. Thanks for following up!\n"},{"id":"557512","messageId":"xmqqfqxgyzfo.fsf@gitster.g","threadId":"66760","inReplyTo":"20261103212410.1389988-2-jltobler@gmail.com","subject":"Re: [PATCH 1/2] object-file: warn on failed unlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-04T23:00:43Z","receivedAt":"2026-11-04T23:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> When the \"files\" ODB transaction cleans up a temporary packfile or\n> \".keep\" lockfile on an error path, any failure to unlink the file is\n> silently ignored. A \".keep\" file left behind this way prevents its\n> packfile from ever being removed by housekeeping.\n>\n> Use `unlink_or_warn()` instead so that such failures are reported.\n>\n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n>  object-file.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 132aaab6f8..a9800143bd 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -823,7 +823,7 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction\n>  \tif (state->nr_written == 0) {\n>  \t\tclose(state->f->fd);\n>  \t\tfree_hashfile(state->f);\n> -\t\tunlink(state->pack_tmp_name);\n> +\t\tunlink_or_warn(state->pack_tmp_name);\n>  \t\tgoto clear_exit;\n>  \t} else if (state->nr_written == 1) {\n>  \t\tfinalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,\n> @@ -1276,13 +1276,13 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,\n>  \tif (write_in_full(fd, contents.buf, contents.len) < 0) {\n>  \t\tret = error_errno(_(\"unable to write '%s'\"), lock_path.buf);\n>  \t\tclose(fd);\n> -\t\tunlink(lock_path.buf);\n> +\t\tunlink_or_warn(lock_path.buf);\n>  \t\tgoto out;\n>  \t}\n>  \n>  \tif (close(fd) < 0) {\n>  \t\tret = error_errno(_(\"unable to close '%s'\"), lock_path.buf);\n> -\t\tunlink(lock_path.buf);\n> +\t\tunlink_or_warn(lock_path.buf);\n>  \t\tgoto out;\n>  \t}\n\nTrivially correct.\n\n"},{"id":"557513","messageId":"xmqq8q38yz5i.fsf@gitster.g","threadId":"66760","inReplyTo":"20261103212410.1389988-3-jltobler@gmail.com","subject":"Re: [PATCH 2/2] object-file: return error on index-pack abnormal exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-04T23:06:49Z","receivedAt":"2026-11-04T23:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> diff --git a/object-file.c b/object-file.c\n> index a9800143bd..e04ce0bc11 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1429,8 +1429,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n>  \tstruct pack_header hdr;\n>  \tconst char *hdr_err;\n>  \tint err_fd = opts->err_fd;\n> -\tint ret = -1;\n> -\tint status;\n> +\tint ret;\n>  \n>  \thdr_err = parse_pack_header(&hdr, pack_fd);\n>  \tif (hdr_err) {\n> @@ -1462,12 +1461,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n>  \t\tchild.in = pack_fd;\n>  \t\tchild.err = err_fd;\n>  \t\tchild.git_cmd = 1;\n> -\t\tstatus = run_command(&child);\n> -\t\tif (status) {\n> +\t\tif (run_command(&child)) {\n>  \t\t\tstrbuf_addstr(err_msg, \"unpack-objects abnormal exit\");\n>  \t\t\treturn -1;\n>  \t\t}\n\nOK.  A slight simplication that does not change any behaviour.\n\n>  \t} else {\n> +\t\tint report_err;\n> +\n>  \t\tstrvec_pushl(&child.args, \"index-pack\", \"--stdin\", NULL);\n>  \t\tpush_header_arg(&child.args, &hdr);\n>  \n> @@ -1487,24 +1487,24 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n>  \t\tchild.in = pack_fd;\n>  \t\tchild.err = err_fd;\n>  \t\tchild.git_cmd = 1;\n> -\t\tstatus = start_command(&child);\n> -\t\tif (status) {\n> +\t\tif (start_command(&child)) {\n>  \t\t\tstrbuf_addstr(err_msg, \"index-pack fork failed\");\n>  \t\t\treturn -1;\n>  \t\t}\n>  \n> -\t\tret = read_index_pack_report(repo, child.out, &pack_name, NULL);\n> +\t\treport_err = read_index_pack_report(repo, child.out, &pack_name, NULL);\n>  \n>  \t\tclose(child.out);\n>  \n> -\t\tstatus = finish_command(&child);\n> -\t\tif (status) {\n> +\t\tif (finish_command(&child)) {\n>  \t\t\tstrbuf_addstr(err_msg, \"index-pack abnormal exit\");\n> +\t\t\tret = -1;\n>  \t\t\tgoto out;\n>  \t\t}\n\nOK.  A slight simplication that does not change any behaviour again.\n\n> -\t\tif (ret) {\n> +\t\tif (report_err) {\n>  \t\t\tstrbuf_addstr(err_msg, \"index-pack failed to report pack name\");\n> +\t\t\tret = -1;\n>  \t\t\tgoto out;\n>  \t\t}\n\nI guess this also does not change a behaviour?  We used to relay the\nerror status returned by read_index_pack_report(), but now we force\nthe error code to an explicit -1.\n\nPersonally, I somehow find that the original \"unless we positively\ndeclare success, default to 'ret = -1', and if we may have detected\nmore specific error, return it back, and reset ret to 0 when we see\na success\" easier to follow, but that may be subjective preference.\n\nWill queue.  Thanks.\n\n"}]}