{"thread":{"id":"57776","subject":"[PATCH 0/4] Fix issues and a regression noted by valgrind","startedAt":"2022-04-21T20:14:45Z","lastAt":"2022-05-20T04:27:39Z","messageCount":20,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"454168","messageId":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","threadId":"57776","inReplyTo":null,"subject":"[PATCH 0/4] Fix issues and a regression noted by valgrind","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T20:14:33Z","receivedAt":"2022-04-21T20:14:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I spotted these when testing v2.36.0, but waited until after the\nrelease since there's no v2.36.0 regressions here.\n\nThe scariest issue here by far is one I caused and am now fixing in\n4/4. As noted I don't think it had any real practical fallout, but we\nshould obviously fix it sooner than later.\n\nI think the 3/4 issue is likewise just a bit scary, but I haven't\nanalyzed it as much. In that case we'd pass uninitialized memory to a\nsyscall.\n\nIt's clear that the --valgrind mode for the test suite is useful, but\nit's sooo sloooow (it takes around a full day to complete, and for me\nthe full tests otherwise run in ~2 minutes). I'll try to run it in the\nbackground with some regularity going forward anyway.\n\nAside: I do see that there's occasionaly FreeBSD runs in the \"main\"\ngit/git CI. How are those hooked up? Running it in GitHub CI would\ntake approximately forever (I think it would run into a hard timeout),\nbut if we can hook up a CI runner on some dedicated hardware.\n\nÆvar Arnfjörð Bjarmason (4):\n  tests: make RUNTIME_PREFIX compatible with --valgrind\n  log test: skip a failing mkstemp() test under valgrind\n  commit-graph.c: don't assume that stat() succeeds\n  object-file: fix a unpack_loose_header() regression in 3b6a8db3b03\n\n commit-graph.c        |  6 ++++--\n object-file.c         |  8 ++++++--\n t/t0060-path-utils.sh |  4 ++--\n t/t1006-cat-file.sh   | 10 ++++++++--\n t/t1450-fsck.sh       | 13 +++++++++++--\n t/t4202-log.sh        | 11 +++++++----\n t/test-lib.sh         |  1 +\n 7 files changed, 39 insertions(+), 14 deletions(-)\n\n-- \n2.36.0.879.gd068ac2c328\n\n"},{"id":"454169","messageId":"patch-1.4-315da7c2df0-20220421T200733Z-avarab@gmail.com","threadId":"57776","inReplyTo":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","subject":"[PATCH 1/4] tests: make RUNTIME_PREFIX compatible with --valgrind","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T20:14:34Z","receivedAt":"2022-04-21T20:14:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in b7d11a0f5d2 (tests: exercise the RUNTIME_PREFIX\nfeature, 2021-07-24) where tests that want to set up and test a \"git\"\nwrapper in $PATH conflicted with the t/bin/valgrind wrapper(s) doing\nthe same.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0060-path-utils.sh | 4 ++--\n t/test-lib.sh         | 1 +\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 2fe6ae6a4e5..aa35350b6f3 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -542,7 +542,7 @@ test_lazy_prereq CAN_EXEC_IN_PWD '\n \t./git rev-parse\n '\n \n-test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n+test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n \tmkdir -p pretend/bin pretend/libexec/git-core &&\n \techo \"echo HERE\" | write_script pretend/libexec/git-core/git-here &&\n \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n@@ -550,7 +550,7 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n \techo HERE >expect &&\n \ttest_cmp expect actual'\n \n-test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n+test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n \tmkdir -p pretend/bin &&\n \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n \tgit config yes.path \"%(prefix)/yes\" &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 531cef097db..7f3d323e937 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1666,6 +1666,7 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq PCRE\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n test -n \"$SANITIZE_LEAK\" && test_set_prereq SANITIZE_LEAK\n+test -n \"$GIT_VALGRIND_ENABLED\" && test_set_prereq VALGRIND\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n-- \n2.36.0.879.gd068ac2c328\n\n"},{"id":"454170","messageId":"patch-2.4-0f7d2622541-20220421T200733Z-avarab@gmail.com","threadId":"57776","inReplyTo":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","subject":"[PATCH 2/4] log test: skip a failing mkstemp() test under valgrind","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T20:14:35Z","receivedAt":"2022-04-21T20:14:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Skip a test added in f1e3df31699 (t: increase test coverage of\nsignature verification output, 2020-03-04) when running under\nvalgrind. Due to valgrind's interception of mkstemp() this test will\nfail with:\n\n\t+ pwd\n\t+ TMPDIR=[...]/t/trash directory.t4202-log/bogus git log --show-signature -n1 plain-fail\n\t==7696== VG_(mkstemp): failed to create temp file: [...]/t/trash directory.t4202-log/bogus/valgrind_proc_7696_cmdline_d545ddcf\n\t[... 10 more similar lines omitted ..]\n\tvalgrind: Startup or configuration error:\n\tvalgrind:    Can't create client cmdline file in [...]/t/trash directory.t4202-log/bogus/valgrind_proc_7696_cmdline_6e542d1d\n\tvalgrind: Unable to start up properly.  Giving up.\n\terror: last command exited with $?=1\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t4202-log.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex be07407f855..6e663525582 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1992,10 +1992,13 @@ test_expect_success GPG 'log --show-signature for merged tag with GPG failure' '\n \tgit tag -s -m signed_tag_msg signed_tag_fail &&\n \tgit checkout plain-fail &&\n \tgit merge --no-ff -m msg signed_tag_fail &&\n-\tTMPDIR=\"$(pwd)/bogus\" git log --show-signature -n1 plain-fail >actual &&\n-\tgrep \"^merged tag\" actual &&\n-\tgrep \"^No signature\" actual &&\n-\t! grep \"^gpg: Signature made\" actual\n+\tif ! test_have_prereq VALGRIND\n+\tthen\n+\t\tTMPDIR=\"$(pwd)/bogus\" git log --show-signature -n1 plain-fail >actual &&\n+\t\tgrep \"^merged tag\" actual &&\n+\t\tgrep \"^No signature\" actual &&\n+\t\t! grep \"^gpg: Signature made\" actual\n+\tfi\n '\n \n test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n-- \n2.36.0.879.gd068ac2c328\n\n"},{"id":"454171","messageId":"patch-3.4-fadaa08a3ff-20220421T200733Z-avarab@gmail.com","threadId":"57776","inReplyTo":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","subject":"[PATCH 3/4] commit-graph.c: don't assume that stat() succeeds","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T20:14:36Z","receivedAt":"2022-04-21T20:14:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix code added in 8d84097f965 (commit-graph: expire commit-graph\nfiles, 2019-06-18) to check the return value of the stat() system\ncall. Not doing so caused us to use uninitialized memory in the \"Bloom\ngeneration is limited by --max-new-filters\" test in\nt4216-log-bloom.sh:\n\n\t+ rm -f trace.event\n\t+ pwd\n\t+ GIT_TRACE2_EVENT=[...]/t/trash directory.t4216-log-bloom/limits/trace.event git commit-graph write --reachable --split=replace --changed-paths --max-new-filters=2\n\t==24835== Syscall param utimensat(times[0].tv_sec) points to uninitialised byte(s)\n\t==24835==    at 0x499E65A: __utimensat64_helper (utimensat.c:34)\n\t==24835==    by 0x4999142: utime (utime.c:36)\n\t==24835==    by 0x552BE0: mark_commit_graphs (commit-graph.c:2213)\n\t==24835==    by 0x550822: write_commit_graph (commit-graph.c:2424)\n\t==24835==    by 0x54E3A0: write_commit_graph_reachable (commit-graph.c:1681)\n\t==24835==    by 0x4374BB: graph_write (commit-graph.c:269)\n\t==24835==    by 0x436F7D: cmd_commit_graph (commit-graph.c:326)\n\t==24835==    by 0x407B9A: run_builtin (git.c:465)\n\t==24835==    by 0x406651: handle_builtin (git.c:719)\n\t==24835==    by 0x407575: run_argv (git.c:786)\n\t==24835==    by 0x406410: cmd_main (git.c:917)\n\t==24835==    by 0x511F09: main (common-main.c:56)\n\t==24835==  Address 0x1ffeffde70 is on thread 1's stack\n\t==24835==  in frame #1, created by utime (utime.c:25)\n\t==24835==  Uninitialised value was created by a stack allocation\n\t==24835==    at 0x552B50: mark_commit_graphs (commit-graph.c:2201)\n\t==24835==\n\t[...]\n\terror: last command exited with $?=126\n\tnot ok 137 - Bloom generation is limited by --max-new-filters\n\nThis would happen as we stat'd the non-existing\n\".git/objects/info/commit-graph\" file. Let's fix mark_commit_graphs()\nto check the statu() return value, and while we're at it fix another\ncase added in the same commit to do the same.\n\nThe caller in expire_commit_graphs() would have been less likely to\nrun into this, as it's operating on files it just got from readdir(),\nbut it could still happen due to a race with e.g. a concurrent \"rm\n-rf\" of the commit-graph files.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n commit-graph.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 441b36016ba..2b528187316 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2206,7 +2206,8 @@ static void mark_commit_graphs(struct write_commit_graph_context *ctx)\n \t\tstruct stat st;\n \t\tstruct utimbuf updated_time;\n \n-\t\tstat(ctx->commit_graph_filenames_before[i], &st);\n+\t\tif (stat(ctx->commit_graph_filenames_before[i], &st) < 0)\n+\t\t\tcontinue;\n \n \t\tupdated_time.actime = st.st_atime;\n \t\tupdated_time.modtime = now;\n@@ -2247,7 +2248,8 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n \n-\t\tstat(path.buf, &st);\n+\t\tif (stat(path.buf, &st) < 0)\n+\t\t\tcontinue;\n \n \t\tif (st.st_mtime > expire_time)\n \t\t\tcontinue;\n-- \n2.36.0.879.gd068ac2c328\n\n"},{"id":"454172","messageId":"patch-4.4-7698c0f11a8-20220421T200733Z-avarab@gmail.com","threadId":"57776","inReplyTo":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","subject":"[PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T20:14:37Z","receivedAt":"2022-04-21T20:14:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in my 3b6a8db3b03 (object-file.c: use \"enum\" return\ntype for unpack_loose_header(), 2021-10-01) revealed both by running\nthe test suite with --valgrind, and with the amended \"git fsck\" test.\n\nIn practice this regression in v2.34.0 caused us to claim that we\ncouldn't parse the header, as opposed to not being able to unpack\nit. Before the change in the C code the test_cmp added here would emit:\n\n\t-error: unable to unpack header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\t+error: unable to parse header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\nI.e. we'd proceed to call parse_loose_header() on the uninitialized\n\"hdr\" value, and it would have been very unlikely for that\nuninitialized memory to be a valid git object.\n\nThe other callers of unpack_loose_header() were already checking the\nenum values exhaustively. See 3b6a8db3b03 and\n5848fb11acd (object-file.c: return ULHR_TOO_LONG on \"header too long\",\n2021-10-01).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n object-file.c       |  8 ++++++--\n t/t1006-cat-file.sh | 10 ++++++++--\n t/t1450-fsck.sh     | 13 +++++++++++--\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5ffbf3d4fd4..b5d1d12b68a 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,\n \t\tgoto out;\n \t}\n \n-\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n-\t\t\t\tNULL) < 0) {\n+\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n+\t\t\t\t    NULL)) {\n+\tcase ULHR_OK:\n+\t\tbreak;\n+\tcase ULHR_BAD:\n+\tcase ULHR_TOO_LONG:\n \t\terror(_(\"unable to unpack header of %s\"), path);\n \t\tgoto out;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 1b852076944..dadf3b14583 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -681,7 +681,7 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n \n \t\t# Setup and create the empty blob and its path\n \t\tempty_path=$(git rev-parse --git-path objects/$(test_oid_to_path \"$EMPTY_BLOB\")) &&\n-\t\tgit hash-object -w --stdin </dev/null &&\n+\t\tempty_blob=$(git hash-object -w --stdin </dev/null) &&\n \n \t\t# Create another blob and its path\n \t\techo other >other.blob &&\n@@ -722,7 +722,13 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n \t\t# content out as-is. Try to make it zlib-invalid.\n \t\tmv -f other.blob \"$empty_path\" &&\n \t\ttest_must_fail git fsck 2>err.fsck &&\n-\t\tgrep \"^error: inflate: data stream error (\" err.fsck\n+\t\tcat >expect <<-EOF &&\n+\t\terror: inflate: data stream error (incorrect header check)\n+\t\terror: unable to unpack header of ./$empty_path\n+\t\terror: $empty_blob: object corrupt or missing: ./$empty_path\n+\t\tEOF\n+\t\tgrep \"^error: \" err.fsck >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex de50c0ea018..ab7f31f1dcd 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -774,10 +774,19 @@ test_expect_success 'fsck finds problems in duplicate loose objects' '\n \t\t# no \"-d\" here, so we end up with duplicates\n \t\tgit repack &&\n \t\t# now corrupt the loose copy\n-\t\tfile=$(sha1_file \"$(git rev-parse HEAD)\") &&\n+\t\toid=\"$(git rev-parse HEAD)\" &&\n+\t\tfile=$(sha1_file \"$oid\") &&\n \t\trm \"$file\" &&\n \t\techo broken >\"$file\" &&\n-\t\ttest_must_fail git fsck\n+\t\ttest_must_fail git fsck 2>err &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\terror: inflate: data stream error (incorrect header check)\n+\t\terror: unable to unpack header of $file\n+\t\terror: $oid: object corrupt or missing: $file\n+\t\tEOF\n+\t\tgrep \"^error: \" err >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \n-- \n2.36.0.879.gd068ac2c328\n\n"},{"id":"454191","messageId":"xmqqlevydqll.fsf@gitster.g","threadId":"57776","inReplyTo":"patch-1.4-315da7c2df0-20220421T200733Z-avarab@gmail.com","subject":"Re: [PATCH 1/4] tests: make RUNTIME_PREFIX compatible with --valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T22:22:14Z","receivedAt":"2022-04-21T22:22:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Subject: Re: [PATCH 1/4] tests: make RUNTIME_PREFIX compatible with --valgrind\n\nThe patch text looks more like \n\n\ttests: --valgrind does not work with RUNTIME_PREFIX\n\nto me.  Is it true that a test that only works with RUNTIME_PREFIX\ndoes not work under --valgrind at all?  I am wondering if we can do\nthis without introducing a new prerequisite, and if it makes sense\nto do so if it can be done.  The output from\n\n    $ git grep 'test_expect_success .*RUNTIME_PREFIX' t/\n\ntells me that there are two test pieces that must be run under\nRUNTIME_PREFIX, and this patch touches both of them.\n\nI guess it is not RUNTIME_PREFIX build itself per-se, but it is\nthe way we test RUNTIME_PREFIX is incompatible with how we run our\ntests under --valgrind, so neither the title on the Subject: header\nof this message or \"looks more like\" above is a good one.  It is\nmore like\n\n\ttests: using custom GIT_EXEC_PATH breaks --valgrind tests\n\nI think.\n\nAnd after having looked at the patch text and thought about the\nissues enough to come up with the above updated title, I can say\nthat the change looks quite reasonable, both the body of the\nproposed log message and the solution.  Nicely done.\n\nIt would have been even nicer if I didn't have to think about the\nissues myself, though ;-)\n\nThanks.\n\n> Fix a regression in b7d11a0f5d2 (tests: exercise the RUNTIME_PREFIX\n> feature, 2021-07-24) where tests that want to set up and test a \"git\"\n> wrapper in $PATH conflicted with the t/bin/valgrind wrapper(s) doing\n> the same.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/t0060-path-utils.sh | 4 ++--\n>  t/test-lib.sh         | 1 +\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n> index 2fe6ae6a4e5..aa35350b6f3 100755\n> --- a/t/t0060-path-utils.sh\n> +++ b/t/t0060-path-utils.sh\n> @@ -542,7 +542,7 @@ test_lazy_prereq CAN_EXEC_IN_PWD '\n>  \t./git rev-parse\n>  '\n>  \n> -test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n> +test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n>  \tmkdir -p pretend/bin pretend/libexec/git-core &&\n>  \techo \"echo HERE\" | write_script pretend/libexec/git-core/git-here &&\n>  \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n> @@ -550,7 +550,7 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n>  \techo HERE >expect &&\n>  \ttest_cmp expect actual'\n>  \n> -test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n> +test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n>  \tmkdir -p pretend/bin &&\n>  \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n>  \tgit config yes.path \"%(prefix)/yes\" &&\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 531cef097db..7f3d323e937 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1666,6 +1666,7 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq PCRE\n>  test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n>  test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n>  test -n \"$SANITIZE_LEAK\" && test_set_prereq SANITIZE_LEAK\n> +test -n \"$GIT_VALGRIND_ENABLED\" && test_set_prereq VALGRIND\n>  \n>  if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n>  then\n"},{"id":"454192","messageId":"xmqqee1qdq97.fsf@gitster.g","threadId":"57776","inReplyTo":"patch-3.4-fadaa08a3ff-20220421T200733Z-avarab@gmail.com","subject":"Re: [PATCH 3/4] commit-graph.c: don't assume that stat() succeeds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T22:29:40Z","receivedAt":"2022-04-21T22:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix code added in 8d84097f965 (commit-graph: expire commit-graph\n> files, 2019-06-18) to check the return value of the stat() system\n> call. Not doing so caused us to use uninitialized memory in the \"Bloom\n> generation is limited by --max-new-filters\" test in\n> t4216-log-bloom.sh:\n>\n> \t+ rm -f trace.event\n> \t+ pwd\n> \t+ GIT_TRACE2_EVENT=[...]/t/trash directory.t4216-log-bloom/limits/trace.event git commit-graph write --reachable --split=replace --changed-paths --max-new-filters=2\n> \t==24835== Syscall param utimensat(times[0].tv_sec) points to uninitialised byte(s)\n> \t==24835==    at 0x499E65A: __utimensat64_helper (utimensat.c:34)\n> \t==24835==    by 0x4999142: utime (utime.c:36)\n> \t==24835==    by 0x552BE0: mark_commit_graphs (commit-graph.c:2213)\n> \t==24835==    by 0x550822: write_commit_graph (commit-graph.c:2424)\n> \t==24835==    by 0x54E3A0: write_commit_graph_reachable (commit-graph.c:1681)\n> \t==24835==    by 0x4374BB: graph_write (commit-graph.c:269)\n> \t==24835==    by 0x436F7D: cmd_commit_graph (commit-graph.c:326)\n> \t==24835==    by 0x407B9A: run_builtin (git.c:465)\n> \t==24835==    by 0x406651: handle_builtin (git.c:719)\n> \t==24835==    by 0x407575: run_argv (git.c:786)\n> \t==24835==    by 0x406410: cmd_main (git.c:917)\n> \t==24835==    by 0x511F09: main (common-main.c:56)\n> \t==24835==  Address 0x1ffeffde70 is on thread 1's stack\n> \t==24835==  in frame #1, created by utime (utime.c:25)\n> \t==24835==  Uninitialised value was created by a stack allocation\n> \t==24835==    at 0x552B50: mark_commit_graphs (commit-graph.c:2201)\n> \t==24835==\n> \t[...]\n> \terror: last command exited with $?=126\n> \tnot ok 137 - Bloom generation is limited by --max-new-filters\n>\n> This would happen as we stat'd the non-existing\n> \".git/objects/info/commit-graph\" file. Let's fix mark_commit_graphs()\n> to check the statu() return value, and while we're at it fix another\n\n\"statu\" -> \"stat\".\n\n> case added in the same commit to do the same.\n>\n> The caller in expire_commit_graphs() would have been less likely to\n> run into this, as it's operating on files it just got from readdir(),\n> but it could still happen due to a race with e.g. a concurrent \"rm\n> -rf\" of the commit-graph files.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  commit-graph.c | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 441b36016ba..2b528187316 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2206,7 +2206,8 @@ static void mark_commit_graphs(struct write_commit_graph_context *ctx)\n>  \t\tstruct stat st;\n>  \t\tstruct utimbuf updated_time;\n>  \n> -\t\tstat(ctx->commit_graph_filenames_before[i], &st);\n> +\t\tif (stat(ctx->commit_graph_filenames_before[i], &st) < 0)\n> +\t\t\tcontinue;\n>  \n>  \t\tupdated_time.actime = st.st_atime;\n>  \t\tupdated_time.modtime = now;\n\nIf we think a commit-graph file should exist and it turns out that\nthe file is gone, we cannot update the file's timestamps, so we need\nto do something about it, but is \"continue\" the most sensible thing,\nI have to wonder?\n\nOf course, missing file is not the only reason not being able to\nstat(); what I am getting at is if this should be warned to the\nuser, or perhaps something like \"warn unless ENOENT\".\n\n> @@ -2247,7 +2248,8 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n>  \t\tstrbuf_setlen(&path, dirnamelen);\n>  \t\tstrbuf_addstr(&path, de->d_name);\n>  \n> -\t\tstat(path.buf, &st);\n> +\t\tif (stat(path.buf, &st) < 0)\n> +\t\t\tcontinue;\n\nDitto about \"should we warn\".\n\n>  \t\tif (st.st_mtime > expire_time)\n>  \t\t\tcontinue;\n\nBut just \"continue\" is the change with least impact.  In either of\nthese codepaths, we blindly continued and ran utime() or unlink()\nwithout checking their return status, but we now refrain from\ntouching them when we cannot stat().\n\nSo, I guess the only actionable thing that needs fixing in this\npatch is the \"statu\" typo.\n\nThanks.\n"},{"id":"454194","messageId":"xmqqa6cedpt3.fsf@gitster.g","threadId":"57776","inReplyTo":"patch-4.4-7698c0f11a8-20220421T200733Z-avarab@gmail.com","subject":"Re: [PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T22:39:20Z","receivedAt":"2022-04-21T22:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> -\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> -\t\t\t\tNULL) < 0) {\n> +\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> +\t\t\t\t    NULL)) {\n> +\tcase ULHR_OK:\n> +\t\tbreak;\n> +\tcase ULHR_BAD:\n> +\tcase ULHR_TOO_LONG:\n>  \t\terror(_(\"unable to unpack header of %s\"), path);\n>  \t\tgoto out;\n>  \t}\n\nSigh, well spotted.  This is why I hate the application of \"enum is\nbetter, let's rewrite the 'negative is error, 0 is good' with it\"\nand other dogmatic \"clean-up\" that touch everywhere in the codebase.\n\nNow because it is ULHR_OK or everything else that is an error, I think\nthe fix should be\n\n\tif (unpack_loose_header(...) != ULHR_OK) {\n\t\terror(...);\n\t\tgoto out;\n\t}\n\nIt would also be much closer in spirit to the original code before\nthe \"enum\" change broke it.\n"},{"id":"454228","messageId":"220422.86levx5xuc.gmgdl@evledraar.gmail.com","threadId":"57776","inReplyTo":"xmqqa6cedpt3.fsf@gitster.g","subject":"Re: [PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-22T08:21:50Z","receivedAt":"2022-04-22T08:25:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 21 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> -\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n>> -\t\t\t\tNULL) < 0) {\n>> +\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n>> +\t\t\t\t    NULL)) {\n>> +\tcase ULHR_OK:\n>> +\t\tbreak;\n>> +\tcase ULHR_BAD:\n>> +\tcase ULHR_TOO_LONG:\n>>  \t\terror(_(\"unable to unpack header of %s\"), path);\n>>  \t\tgoto out;\n>>  \t}\n>\n> Sigh, well spotted.  This is why I hate the application of \"enum is\n> better, let's rewrite the 'negative is error, 0 is good' with it\"\n> and other dogmatic \"clean-up\" that touch everywhere in the codebase.\n\nWhile this is squarely my fault, I'm FWIW not as dogmatic on that point\nas you think. I initially made a new error state a -2, and got feedback\non the series that that was too magical, then ended up turning it into\nan enum and missed this callsite.\n\nI think it's less that enums are bad in this case, as it's probably\nsensible to consistently use negative values for error states.\n\n> Now because it is ULHR_OK or everything else that is an error, I think\n> the fix should be\n>\n> \tif (unpack_loose_header(...) != ULHR_OK) {\n> \t\terror(...);\n> \t\tgoto out;\n> \t}\n>\n> It would also be much closer in spirit to the original code before\n> the \"enum\" change broke it.\n\nWe have two other callers of the API using the exhaustive enumeration\npattern, so by doing this we'd have the compiler miss this callsite if\nanother label is added.\n\nIt could be refactored etc., but I think this change as-is is the most\nminimal & least invasive. I.e. it just adjusts this one caller to match\nthe other ones, we could also refactor the interface & pattern we use to\ncall it.\n\nBut if we're doing that I don't see the benefit of doing it for just one\ncaller, and if we're doing it for all of them surely that's better as\nsome follow-up cleanup...\n"},{"id":"455223","messageId":"20220512223218.237544-2-gitster@pobox.com","threadId":"57776","inReplyTo":"20220512223218.237544-1-gitster@pobox.com","subject":"[PATCH v2 1/4] tests: using custom GIT_EXEC_PATH breaks --valgrind tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:32:15Z","receivedAt":"2022-05-12T22:32:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nFix a regression in b7d11a0f5d2 (tests: exercise the RUNTIME_PREFIX\nfeature, 2021-07-24) where tests that want to set up and test a \"git\"\nwrapper in $PATH conflicted with the t/bin/valgrind wrapper(s) doing\nthe same.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t0060-path-utils.sh | 4 ++--\n t/test-lib.sh         | 1 +\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 2fe6ae6a4e..aa35350b6f 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -542,7 +542,7 @@ test_lazy_prereq CAN_EXEC_IN_PWD '\n \t./git rev-parse\n '\n \n-test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n+test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n \tmkdir -p pretend/bin pretend/libexec/git-core &&\n \techo \"echo HERE\" | write_script pretend/libexec/git-core/git-here &&\n \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n@@ -550,7 +550,7 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '\n \techo HERE >expect &&\n \ttest_cmp expect actual'\n \n-test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n+test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '\n \tmkdir -p pretend/bin &&\n \tcp \"$GIT_EXEC_PATH\"/git$X pretend/bin/ &&\n \tgit config yes.path \"%(prefix)/yes\" &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 531cef097d..7f3d323e93 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1666,6 +1666,7 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq PCRE\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n test -n \"$SANITIZE_LEAK\" && test_set_prereq SANITIZE_LEAK\n+test -n \"$GIT_VALGRIND_ENABLED\" && test_set_prereq VALGRIND\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n-- \n2.36.1-338-g1c7f76a54c\n\n"},{"id":"455224","messageId":"20220512223218.237544-3-gitster@pobox.com","threadId":"57776","inReplyTo":"20220512223218.237544-1-gitster@pobox.com","subject":"[PATCH v2 2/4] log test: skip a failing mkstemp() test under valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:32:16Z","receivedAt":"2022-05-12T22:32:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nSkip a test added in f1e3df31699 (t: increase test coverage of\nsignature verification output, 2020-03-04) when running under\nvalgrind. Due to valgrind's interception of mkstemp() this test will\nfail with:\n\n\t+ pwd\n\t+ TMPDIR=[...]/t/trash directory.t4202-log/bogus git log --show-signature -n1 plain-fail\n\t==7696== VG_(mkstemp): failed to create temp file: [...]/t/trash directory.t4202-log/bogus/valgrind_proc_7696_cmdline_d545ddcf\n\t[... 10 more similar lines omitted ..]\n\tvalgrind: Startup or configuration error:\n\tvalgrind:    Can't create client cmdline file in [...]/t/trash directory.t4202-log/bogus/valgrind_proc_7696_cmdline_6e542d1d\n\tvalgrind: Unable to start up properly.  Giving up.\n\terror: last command exited with $?=1\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4202-log.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex be07407f85..6e66352558 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1992,10 +1992,13 @@ test_expect_success GPG 'log --show-signature for merged tag with GPG failure' '\n \tgit tag -s -m signed_tag_msg signed_tag_fail &&\n \tgit checkout plain-fail &&\n \tgit merge --no-ff -m msg signed_tag_fail &&\n-\tTMPDIR=\"$(pwd)/bogus\" git log --show-signature -n1 plain-fail >actual &&\n-\tgrep \"^merged tag\" actual &&\n-\tgrep \"^No signature\" actual &&\n-\t! grep \"^gpg: Signature made\" actual\n+\tif ! test_have_prereq VALGRIND\n+\tthen\n+\t\tTMPDIR=\"$(pwd)/bogus\" git log --show-signature -n1 plain-fail >actual &&\n+\t\tgrep \"^merged tag\" actual &&\n+\t\tgrep \"^No signature\" actual &&\n+\t\t! grep \"^gpg: Signature made\" actual\n+\tfi\n '\n \n test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n-- \n2.36.1-338-g1c7f76a54c\n\n"},{"id":"455225","messageId":"20220512223218.237544-1-gitster@pobox.com","threadId":"57776","inReplyTo":"cover-0.4-00000000000-20220421T200733Z-avarab@gmail.com","subject":"[PATCH v2 0/4] test fixes around valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:32:14Z","receivedAt":"2022-05-12T22:36:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Updates the valgrind test framework a bit, and then fixes a few\nissues valgrind has found.\n\nEssentailly the same as the previously posted version, but those\nsuggested fixes squashed in.\n\nI am sending this out as a final \"complain now, or this will go to\n'next' soonish\" warning.  The \"What's cooking\" report is getting\ncrowded with too many topics marked as \"Expecting a reroll\", and I'm\ntrying to do easier ones myself to see how much reduction we can\nmake.\n\nÆvar Arnfjörð Bjarmason (4):\n  tests: using custom GIT_EXEC_PATH breaks --valgrind tests\n  log test: skip a failing mkstemp() test under valgrind\n  commit-graph.c: don't assume that stat() succeeds\n  object-file: fix a unpack_loose_header() regression in 3b6a8db3b03\n\n commit-graph.c        |  6 ++++--\n object-file.c         |  8 ++++++--\n t/t0060-path-utils.sh |  4 ++--\n t/t1006-cat-file.sh   | 10 ++++++++--\n t/t1450-fsck.sh       | 13 +++++++++++--\n t/t4202-log.sh        | 11 +++++++----\n t/test-lib.sh         |  1 +\n 7 files changed, 39 insertions(+), 14 deletions(-)\n\n\n1:  4ebaddb01d ! 1:  3b79af1dde tests: make RUNTIME_PREFIX compatible with --valgrind\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    tests: make RUNTIME_PREFIX compatible with --valgrind\n    +    tests: using custom GIT_EXEC_PATH breaks --valgrind tests\n     \n         Fix a regression in b7d11a0f5d2 (tests: exercise the RUNTIME_PREFIX\n         feature, 2021-07-24) where tests that want to set up and test a \"git\"\n2:  b25609d6fc = 2:  35a7226706 log test: skip a failing mkstemp() test under valgrind\n3:  d56edb6654 ! 3:  84d1793178 commit-graph.c: don't assume that stat() succeeds\n    @@ Commit message\n     \n         This would happen as we stat'd the non-existing\n         \".git/objects/info/commit-graph\" file. Let's fix mark_commit_graphs()\n    -    to check the statu() return value, and while we're at it fix another\n    +    to check the stat()'s return value, and while we're at it fix another\n         case added in the same commit to do the same.\n     \n         The caller in expire_commit_graphs() would have been less likely to\n4:  995cfb0439 = 4:  391ae0a276 object-file: fix a unpack_loose_header() regression in 3b6a8db3b03\n\n\n\n-- \n2.36.1-338-g1c7f76a54c\n\n"},{"id":"455226","messageId":"20220512223218.237544-5-gitster@pobox.com","threadId":"57776","inReplyTo":"20220512223218.237544-1-gitster@pobox.com","subject":"[PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:32:18Z","receivedAt":"2022-05-12T22:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nFix a regression in my 3b6a8db3b03 (object-file.c: use \"enum\" return\ntype for unpack_loose_header(), 2021-10-01) revealed both by running\nthe test suite with --valgrind, and with the amended \"git fsck\" test.\n\nIn practice this regression in v2.34.0 caused us to claim that we\ncouldn't parse the header, as opposed to not being able to unpack\nit. Before the change in the C code the test_cmp added here would emit:\n\n\t-error: unable to unpack header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\t+error: unable to parse header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\nI.e. we'd proceed to call parse_loose_header() on the uninitialized\n\"hdr\" value, and it would have been very unlikely for that\nuninitialized memory to be a valid git object.\n\nThe other callers of unpack_loose_header() were already checking the\nenum values exhaustively. See 3b6a8db3b03 and\n5848fb11acd (object-file.c: return ULHR_TOO_LONG on \"header too long\",\n2021-10-01).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n object-file.c       |  8 ++++++--\n t/t1006-cat-file.sh | 10 ++++++++--\n t/t1450-fsck.sh     | 13 +++++++++++--\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5ffbf3d4fd..b5d1d12b68 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,\n \t\tgoto out;\n \t}\n \n-\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n-\t\t\t\tNULL) < 0) {\n+\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n+\t\t\t\t    NULL)) {\n+\tcase ULHR_OK:\n+\t\tbreak;\n+\tcase ULHR_BAD:\n+\tcase ULHR_TOO_LONG:\n \t\terror(_(\"unable to unpack header of %s\"), path);\n \t\tgoto out;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 1b85207694..dadf3b1458 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -681,7 +681,7 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n \n \t\t# Setup and create the empty blob and its path\n \t\tempty_path=$(git rev-parse --git-path objects/$(test_oid_to_path \"$EMPTY_BLOB\")) &&\n-\t\tgit hash-object -w --stdin </dev/null &&\n+\t\tempty_blob=$(git hash-object -w --stdin </dev/null) &&\n \n \t\t# Create another blob and its path\n \t\techo other >other.blob &&\n@@ -722,7 +722,13 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n \t\t# content out as-is. Try to make it zlib-invalid.\n \t\tmv -f other.blob \"$empty_path\" &&\n \t\ttest_must_fail git fsck 2>err.fsck &&\n-\t\tgrep \"^error: inflate: data stream error (\" err.fsck\n+\t\tcat >expect <<-EOF &&\n+\t\terror: inflate: data stream error (incorrect header check)\n+\t\terror: unable to unpack header of ./$empty_path\n+\t\terror: $empty_blob: object corrupt or missing: ./$empty_path\n+\t\tEOF\n+\t\tgrep \"^error: \" err.fsck >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex de50c0ea01..ab7f31f1dc 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -774,10 +774,19 @@ test_expect_success 'fsck finds problems in duplicate loose objects' '\n \t\t# no \"-d\" here, so we end up with duplicates\n \t\tgit repack &&\n \t\t# now corrupt the loose copy\n-\t\tfile=$(sha1_file \"$(git rev-parse HEAD)\") &&\n+\t\toid=\"$(git rev-parse HEAD)\" &&\n+\t\tfile=$(sha1_file \"$oid\") &&\n \t\trm \"$file\" &&\n \t\techo broken >\"$file\" &&\n-\t\ttest_must_fail git fsck\n+\t\ttest_must_fail git fsck 2>err &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\terror: inflate: data stream error (incorrect header check)\n+\t\terror: unable to unpack header of $file\n+\t\terror: $oid: object corrupt or missing: $file\n+\t\tEOF\n+\t\tgrep \"^error: \" err >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \n-- \n2.36.1-338-g1c7f76a54c\n\n"},{"id":"455227","messageId":"20220512223218.237544-4-gitster@pobox.com","threadId":"57776","inReplyTo":"20220512223218.237544-1-gitster@pobox.com","subject":"[PATCH v2 3/4] commit-graph.c: don't assume that stat() succeeds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:32:17Z","receivedAt":"2022-05-12T22:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nFix code added in 8d84097f965 (commit-graph: expire commit-graph\nfiles, 2019-06-18) to check the return value of the stat() system\ncall. Not doing so caused us to use uninitialized memory in the \"Bloom\ngeneration is limited by --max-new-filters\" test in\nt4216-log-bloom.sh:\n\n\t+ rm -f trace.event\n\t+ pwd\n\t+ GIT_TRACE2_EVENT=[...]/t/trash directory.t4216-log-bloom/limits/trace.event git commit-graph write --reachable --split=replace --changed-paths --max-new-filters=2\n\t==24835== Syscall param utimensat(times[0].tv_sec) points to uninitialised byte(s)\n\t==24835==    at 0x499E65A: __utimensat64_helper (utimensat.c:34)\n\t==24835==    by 0x4999142: utime (utime.c:36)\n\t==24835==    by 0x552BE0: mark_commit_graphs (commit-graph.c:2213)\n\t==24835==    by 0x550822: write_commit_graph (commit-graph.c:2424)\n\t==24835==    by 0x54E3A0: write_commit_graph_reachable (commit-graph.c:1681)\n\t==24835==    by 0x4374BB: graph_write (commit-graph.c:269)\n\t==24835==    by 0x436F7D: cmd_commit_graph (commit-graph.c:326)\n\t==24835==    by 0x407B9A: run_builtin (git.c:465)\n\t==24835==    by 0x406651: handle_builtin (git.c:719)\n\t==24835==    by 0x407575: run_argv (git.c:786)\n\t==24835==    by 0x406410: cmd_main (git.c:917)\n\t==24835==    by 0x511F09: main (common-main.c:56)\n\t==24835==  Address 0x1ffeffde70 is on thread 1's stack\n\t==24835==  in frame #1, created by utime (utime.c:25)\n\t==24835==  Uninitialised value was created by a stack allocation\n\t==24835==    at 0x552B50: mark_commit_graphs (commit-graph.c:2201)\n\t==24835==\n\t[...]\n\terror: last command exited with $?=126\n\tnot ok 137 - Bloom generation is limited by --max-new-filters\n\nThis would happen as we stat'd the non-existing\n\".git/objects/info/commit-graph\" file. Let's fix mark_commit_graphs()\nto check the stat()'s return value, and while we're at it fix another\ncase added in the same commit to do the same.\n\nThe caller in expire_commit_graphs() would have been less likely to\nrun into this, as it's operating on files it just got from readdir(),\nbut it could still happen due to a race with e.g. a concurrent \"rm\n-rf\" of the commit-graph files.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n commit-graph.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 441b36016b..2b52818731 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2206,7 +2206,8 @@ static void mark_commit_graphs(struct write_commit_graph_context *ctx)\n \t\tstruct stat st;\n \t\tstruct utimbuf updated_time;\n \n-\t\tstat(ctx->commit_graph_filenames_before[i], &st);\n+\t\tif (stat(ctx->commit_graph_filenames_before[i], &st) < 0)\n+\t\t\tcontinue;\n \n \t\tupdated_time.actime = st.st_atime;\n \t\tupdated_time.modtime = now;\n@@ -2247,7 +2248,8 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n \n-\t\tstat(path.buf, &st);\n+\t\tif (stat(path.buf, &st) < 0)\n+\t\t\tcontinue;\n \n \t\tif (st.st_mtime > expire_time)\n \t\t\tcontinue;\n-- \n2.36.1-338-g1c7f76a54c\n\n"},{"id":"455238","messageId":"xmqqo802codb.fsf@gitster.g","threadId":"57776","inReplyTo":"20220512223218.237544-5-gitster@pobox.com","subject":"Re: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T23:39:12Z","receivedAt":"2022-05-12T23:39:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> diff --git a/object-file.c b/object-file.c\n> index 5ffbf3d4fd..b5d1d12b68 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,\n>  \t\tgoto out;\n>  \t}\n>  \n> -\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> -\t\t\t\tNULL) < 0) {\n> +\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> +\t\t\t\t    NULL)) {\n> +\tcase ULHR_OK:\n> +\t\tbreak;\n> +\tcase ULHR_BAD:\n> +\tcase ULHR_TOO_LONG:\n>  \t\terror(_(\"unable to unpack header of %s\"), path);\n>  \t\tgoto out;\n>  \t}\n\nRegarding this hunk, since we only care about a single \"did we get\nany error, or did we unpack OK\" bit, I think this should be more\nlike\n\n\tif (unpack_loose_header(...) != ULHR_OK) {\n\t\terror(_(\"unable to...\"), path);\n\t\tgoto out;\n\t}\n\nIt is true, as Ævar mentioned, that there is another place in the\nsame file that uses switch() in loose_object_info(), and it should\nremain to be switch() on the returned enum because it wants to\nbehave differnetly depending on the kind of error it gets.  But that\nis not a reason to make this part that only cares about a single\n\"did it fail?\" into a switch and force future developers to add a\nuseless case arm.\n\nI left it there as posted in the previous round because I was too\nlazy ;-) and also it is something we can clean up with a follow up\npatch outside the series.  As my today's focus has been to reduce\nthe number of topics waiting for a reroll, I'd rather leave things\nthat are not outright broken but needs clean up as they are for the\nsake of expediency.\n\nThanks.\n"},{"id":"455320","messageId":"377be0e9-8a0f-4a86-0a66-3b08c0284dae@github.com","threadId":"57776","inReplyTo":"xmqqo802codb.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-16T14:59:27Z","receivedAt":"2022-05-16T14:59:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/12/2022 7:39 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> diff --git a/object-file.c b/object-file.c\n>> index 5ffbf3d4fd..b5d1d12b68 100644\n>> --- a/object-file.c\n>> +++ b/object-file.c\n>> @@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,\n>>  \t\tgoto out;\n>>  \t}\n>>  \n>> -\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n>> -\t\t\t\tNULL) < 0) {\n>> +\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n>> +\t\t\t\t    NULL)) {\n>> +\tcase ULHR_OK:\n>> +\t\tbreak;\n>> +\tcase ULHR_BAD:\n>> +\tcase ULHR_TOO_LONG:\n>>  \t\terror(_(\"unable to unpack header of %s\"), path);\n>>  \t\tgoto out;\n>>  \t}\n> \n> Regarding this hunk, since we only care about a single \"did we get\n> any error, or did we unpack OK\" bit, I think this should be more\n> like\n> \n> \tif (unpack_loose_header(...) != ULHR_OK) {\n> \t\terror(_(\"unable to...\"), path);\n> \t\tgoto out;\n> \t}\n> \n> It is true, as Ævar mentioned, that there is another place in the\n> same file that uses switch() in loose_object_info(), and it should\n> remain to be switch() on the returned enum because it wants to\n> behave differnetly depending on the kind of error it gets.  But that\n> is not a reason to make this part that only cares about a single\n> \"did it fail?\" into a switch and force future developers to add a\n> useless case arm.\n> \n> I left it there as posted in the previous round because I was too\n> lazy ;-) and also it is something we can clean up with a follow up\n> patch outside the series.  As my today's focus has been to reduce\n> the number of topics waiting for a reroll, I'd rather leave things\n> that are not outright broken but needs clean up as they are for the\n> sake of expediency.\n\nTaking a look at your new version, I agree that this use of 'switch'\nis out of place and can make things more confusing in the future.\n\nHere is a patch doing exactly what you recommended, which you can\nchoose to add or squash. I made you co-author, but I expect you to\nadd your sign-off after mine.\n\n-- >8 --\n\nFrom 85cd37b4f23e06980ea95311067d735144fe932f Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee <derrickstolee@github.com>\nDate: Mon, 16 May 2022 10:53:27 -0400\nSubject: [PATCH] object-file: convert 'switch' back to 'if'\n\nThis switch statement was recently added to make it clear that\nunpack_loose_header() returns an enum value, not an int. This adds\ncomplications for future developers if that enum gains new values, since\nthat developer would need to add a case statement to this switch for\nlittle real value.\n\nInstead, we can revert back to an 'if' statement, but make the enum\nexplicit by using \"!= ULHR_OK\" instead of assuming it has the numerical\nvalue zero.\n\nCo-authored-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n\n object-file.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex b5d1d12b68a..52e4ae1b5f0 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2623,12 +2623,8 @@ int read_loose_object(const char *path,\n \t\tgoto out;\n \t}\n \n-\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n-\t\t\t\t    NULL)) {\n-\tcase ULHR_OK:\n-\t\tbreak;\n-\tcase ULHR_BAD:\n-\tcase ULHR_TOO_LONG:\n+\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n+\t\t\t\tNULL) != ULHR_OK) {\n \t\terror(_(\"unable to unpack header of %s\"), path);\n \t\tgoto out;\n \t}\n-- \n2.35.3.vfs.0.0\n\n\n"},{"id":"455533","messageId":"RFC-cover-0.2-00000000000-20220519T195055Z-avarab@gmail.com","threadId":"57776","inReplyTo":"377be0e9-8a0f-4a86-0a66-3b08c0284dae@github.com","subject":"[RFC PATCH 0/2] Alternate ab/valgrind-fixes fix-up","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-19T20:09:15Z","receivedAt":"2022-05-19T20:09:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 16 2022, Derrick Stolee wrote:\n\n> On 5/12/2022 7:39 PM, Junio C Hamano wrote:\n> [...]\n> This switch statement was recently added to make it clear that\n> unpack_loose_header() returns an enum value, not an int. This adds\n> complications for future developers if that enum gains new values, since\n> that developer would need to add a case statement to this switch for\n> little real value.\n>\n> Instead, we can revert back to an 'if' statement, but make the enum\n> explicit by using \"!= ULHR_OK\" instead of assuming it has the numerical\n> value zero.\n>\n> Co-authored-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>\n>  object-file.c | 8 ++------\n>  1 file changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index b5d1d12b68a..52e4ae1b5f0 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -2623,12 +2623,8 @@ int read_loose_object(const char *path,\n>  \t\tgoto out;\n>  \t}\n>  \n> -\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> -\t\t\t\t    NULL)) {\n> -\tcase ULHR_OK:\n> -\t\tbreak;\n> -\tcase ULHR_BAD:\n> -\tcase ULHR_TOO_LONG:\n> +\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n> +\t\t\t\tNULL) != ULHR_OK) {\n>  \t\terror(_(\"unable to unpack header of %s\"), path);\n>  \t\tgoto out;\n>  \t}\n\nThis whole topic-at-large is a stylistic fix-up for a case where I\nobviously got it wrong, so take this with a double grain of salt.\n\nRe the \"What's Cooking\" mention of\nds/object-file-unpack-loose-header-fix: I don't mind it being merged\ndown at all. The below is all small potatoes.\n\nI don't think the rationale (\"adds complications for future\ndevelopers\") makes sense in this case.\n\nLet's leave aside the question of whether we exhaustively check enum\narms as in the pre-image, or check \"not ok\" as in the post-image.\n\nSurely we can agree that whatever pattern is preferred we're better\noff consistently picking one or the other?\n\nI think this proposed change would make more sense and be in line with\nits commit message if it also proposed this:\n\t\n\tdiff --git a/streaming.c b/streaming.c\n\tindex fe54665d86e..bb4ed198463 100644\n\t--- a/streaming.c\n\t+++ b/streaming.c\n\t@@ -230,15 +230,10 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,\n\t \tst->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);\n\t \tif (!st->u.loose.mapped)\n\t \t\treturn -1;\n\t-\tswitch (unpack_loose_header(&st->z, st->u.loose.mapped,\n\t-\t\t\t\t    st->u.loose.mapsize, st->u.loose.hdr,\n\t-\t\t\t\t    sizeof(st->u.loose.hdr), NULL)) {\n\t-\tcase ULHR_OK:\n\t-\t\tbreak;\n\t-\tcase ULHR_BAD:\n\t-\tcase ULHR_TOO_LONG:\n\t+\tif (unpack_loose_header(&st->z, st->u.loose.mapped,\n\t+\t\t\t\tst->u.loose.mapsize, st->u.loose.hdr,\n\t+\t\t\t\tsizeof(st->u.loose.hdr), NULL) != ULHR_OK)\n\t \t\tgoto error;\n\t-\t}\n\t \tif (parse_loose_header(st->u.loose.hdr, &oi) < 0 || *type < 0)\n\t \t\tgoto error;\n\nI.e. now we've converted the 2/3 callers of the API that only cared\nabout \"not OK\", there's a third one that cares about all the enum arms\ncurrently, so that one remains a \"switch\".\n\nThe reason I think the rationale doesn't make sense is because of this\ninconsistency. I.e. if we suppose a developer adds another enum value,\nthey'll then discover those three callers.\n\nSurely whatever our preference for how to handle those 2/3 callers\nit's less complicated if they don't use different patterns for no\nobvious reason.\n\nBut anyway. Looking a bit deeper at this code again I think these two\npatches are where we'd eventually want to head with this API. I.e. I\nthink the whole business of making this a tri-state return was\npremature on my part.\n\nAfter this RFC unpack_loose_header() is again a function that returns\na negative value on error, and the enum is gone. As noted in 2/2\nthere's a slight trade-off there, which I think is for the better,\nboth in terms of API simplicity, and in the new \"error\" output we'll\nomit in these obscure cases. I.e.:\n\n\t-\t\t\t\terror: header for $bogus_long_sha1 too long, exceeds 32 bytes\n\t+\t\t\t\terror: header too long, exceeds 32 bytes\n\t+\t\t\t\terror: unable to unpack $bogus_long_sha1 header\n\nThis whole \"switch\" complexity was because the old error message\nwanted to note the OID in the \"header too long\" message.\n\nAgain, I'm perfectly fine with ds/object-file-unpack-loose-header-fix\nadvancing to \"next\", I can rebase this on top, or drop it depending on\nthe consensus about whether it's worthwile. I did want to un-block\nthat topic one way or the other, so to the extent that it was waiting\non my feedback...\n\nÆvar Arnfjörð Bjarmason (2):\n  object-file API: fix obscure unpack_loose_header() return\n  object-file API: have unpack_loose_header() return \"int\" again\n\n cache.h             | 25 +++++-------------------\n object-file.c       | 46 +++++++++++++++++----------------------------\n streaming.c         | 11 +++--------\n t/t1006-cat-file.sh |  6 ++++--\n 4 files changed, 29 insertions(+), 59 deletions(-)\n\n-- \n2.36.1.957.g2c13267e09b\n\n"},{"id":"455534","messageId":"RFC-patch-1.2-297bea05118-20220519T195055Z-avarab@gmail.com","threadId":"57776","inReplyTo":"RFC-cover-0.2-00000000000-20220519T195055Z-avarab@gmail.com","subject":"[RFC PATCH 1/2] object-file API: fix obscure unpack_loose_header() return","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-19T20:09:16Z","receivedAt":"2022-05-19T20:09:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix an obscure unpack_loose_header() return value. In my\n5848fb11acd (object-file.c: return ULHR_TOO_LONG on \"header too long\",\n2021-10-01) this API learned to return ULHR_TOO_LONG, but should not\nhave done so in the case of parsing a long header where the\nterminating \\0 cannot be found. We should return a ULHR_BAD in that\ncase.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n cache.h       | 3 +--\n object-file.c | 2 +-\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 595582becc8..aa24d5a609f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1342,8 +1342,7 @@ int git_open_cloexec(const char *name, int flags);\n  * \"hdrbuf\" argument is non-NULL. This is intended for use with\n  * OBJECT_INFO_ALLOW_UNKNOWN_TYPE to extract the bad type for (error)\n  * reporting. The full header will be extracted to \"hdrbuf\" for use\n- * with parse_loose_header(), ULHR_TOO_LONG will still be returned\n- * from this function to indicate that the header was too long.\n+ * with parse_loose_header().\n  */\n enum unpack_loose_header_result {\n \tULHR_OK,\ndiff --git a/object-file.c b/object-file.c\nindex b5d1d12b68a..1babe9791f6 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1301,7 +1301,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t\tstream->next_out = buffer;\n \t\tstream->avail_out = bufsiz;\n \t} while (status != Z_STREAM_END);\n-\treturn ULHR_TOO_LONG;\n+\treturn ULHR_BAD;\n }\n \n static void *unpack_loose_rest(git_zstream *stream,\n-- \n2.36.1.957.g2c13267e09b\n\n"},{"id":"455535","messageId":"RFC-patch-2.2-af0dfd017af-20220519T195055Z-avarab@gmail.com","threadId":"57776","inReplyTo":"RFC-cover-0.2-00000000000-20220519T195055Z-avarab@gmail.com","subject":"[RFC PATCH 2/2] object-file API: have unpack_loose_header() return \"int\" again","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-19T20:09:17Z","receivedAt":"2022-05-19T20:09:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Revert the API change in my 5848fb11acd (object-file.c: return\nULHR_TOO_LONG on \"header too long\", 2021-10-01) in favor of having\nunpack_loose_header() return a simple negative value on error.\n\nThe more complex API was needed to give us flexibility that we didn't\nreally need. We only used the ULHR_TOO_LONG return case for headers\nwhich \"cat-file --allow-unknown-type\" is needed for. Let's instead\nerror() on those unconditionally.\n\nThe slight change in the \"error\" message is that we won't include the\nOID in the error in that case, but we will include it in the \"unable\nto unpack\" error emitted by loose_object_info().\n\nAs noted in the preceding commit we were mishandling the case where\nwe'll now error() with \"could not find end of corrupt long header\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n cache.h             | 22 ++++------------------\n object-file.c       | 46 +++++++++++++++++----------------------------\n streaming.c         | 11 +++--------\n t/t1006-cat-file.sh |  6 ++++--\n 4 files changed, 28 insertions(+), 57 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex aa24d5a609f..629c4a84b90 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1330,13 +1330,7 @@ int git_open_cloexec(const char *name, int flags);\n \n /**\n  * unpack_loose_header() initializes the data stream needed to unpack\n- * a loose object header.\n- *\n- * Returns:\n- *\n- * - ULHR_OK on success\n- * - ULHR_BAD on error\n- * - ULHR_TOO_LONG if the header was too long\n+ * a loose object header. A negative value indicates an error.\n  *\n  * It will only parse up to MAX_HEADER_LEN bytes unless an optional\n  * \"hdrbuf\" argument is non-NULL. This is intended for use with\n@@ -1344,17 +1338,9 @@ int git_open_cloexec(const char *name, int flags);\n  * reporting. The full header will be extracted to \"hdrbuf\" for use\n  * with parse_loose_header().\n  */\n-enum unpack_loose_header_result {\n-\tULHR_OK,\n-\tULHR_BAD,\n-\tULHR_TOO_LONG,\n-};\n-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n-\t\t\t\t\t\t    unsigned char *map,\n-\t\t\t\t\t\t    unsigned long mapsize,\n-\t\t\t\t\t\t    void *buffer,\n-\t\t\t\t\t\t    unsigned long bufsiz,\n-\t\t\t\t\t\t    struct strbuf *hdrbuf);\n+int unpack_loose_header(git_zstream *stream, unsigned char *map,\n+\t\t\tunsigned long mapsize, void *buffer,\n+\t\t\tunsigned long bufsiz, struct strbuf *header);\n \n /**\n  * parse_loose_header() parses the starting \"<type> <len>\\0\" of an\ndiff --git a/object-file.c b/object-file.c\nindex 1babe9791f6..1d2901d0e78 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1245,12 +1245,9 @@ void *map_loose_object(struct repository *r,\n \treturn map_loose_object_1(r, NULL, oid, size);\n }\n \n-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n-\t\t\t\t\t\t    unsigned char *map,\n-\t\t\t\t\t\t    unsigned long mapsize,\n-\t\t\t\t\t\t    void *buffer,\n-\t\t\t\t\t\t    unsigned long bufsiz,\n-\t\t\t\t\t\t    struct strbuf *header)\n+int unpack_loose_header(git_zstream *stream, unsigned char *map,\n+\t\t\tunsigned long mapsize, void *buffer,\n+\t\t\tunsigned long bufsiz, struct strbuf *header)\n {\n \tint status;\n \n@@ -1266,13 +1263,13 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \tstatus = git_inflate(stream, 0);\n \tobj_read_lock();\n \tif (status < Z_OK)\n-\t\treturn ULHR_BAD;\n+\t\treturn -1;\n \n \t/*\n \t * Check if entire header is unpacked in the first iteration.\n \t */\n \tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n-\t\treturn ULHR_OK;\n+\t\treturn 0;\n \n \t/*\n \t * We have a header longer than MAX_HEADER_LEN. The \"header\"\n@@ -1280,7 +1277,8 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t * --allow-unknown-type\".\n \t */\n \tif (!header)\n-\t\treturn ULHR_TOO_LONG;\n+\t\treturn error(_(\"header too long, exceeds %d bytes\"),\n+\t\t\t     MAX_HEADER_LEN);\n \n \t/*\n \t * buffer[0..bufsiz] was not large enough.  Copy the partial\n@@ -1301,7 +1299,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t\tstream->next_out = buffer;\n \t\tstream->avail_out = bufsiz;\n \t} while (status != Z_STREAM_END);\n-\treturn ULHR_BAD;\n+\treturn error(_(\"could not find end of corrupt long header\"));\n }\n \n static void *unpack_loose_rest(git_zstream *stream,\n@@ -1466,32 +1464,26 @@ static int loose_object_info(struct repository *r,\n \tif (oi->disk_sizep)\n \t\t*oi->disk_sizep = mapsize;\n \n-\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n-\t\t\t\t    allow_unknown ? &hdrbuf : NULL)) {\n-\tcase ULHR_OK:\n+\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n+\t\t\t\tallow_unknown ? &hdrbuf : NULL) < 0) {\n+\t\tstatus = error(_(\"unable to unpack %s header\"),\n+\t\t\t       oid_to_hex(oid));\n+\t} else {\n \t\tif (parse_loose_header(hdrbuf.len ? hdrbuf.buf : hdr, oi) < 0)\n \t\t\tstatus = error(_(\"unable to parse %s header\"), oid_to_hex(oid));\n \t\telse if (!allow_unknown && *oi->typep < 0)\n \t\t\tdie(_(\"invalid object type\"));\n \n \t\tif (!oi->contentp)\n-\t\t\tbreak;\n+\t\t\tgoto inflate_end;\n \t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n \t\tif (*oi->contentp)\n \t\t\tgoto cleanup;\n \n \t\tstatus = -1;\n-\t\tbreak;\n-\tcase ULHR_BAD:\n-\t\tstatus = error(_(\"unable to unpack %s header\"),\n-\t\t\t       oid_to_hex(oid));\n-\t\tbreak;\n-\tcase ULHR_TOO_LONG:\n-\t\tstatus = error(_(\"header for %s too long, exceeds %d bytes\"),\n-\t\t\t       oid_to_hex(oid), MAX_HEADER_LEN);\n-\t\tbreak;\n \t}\n \n+inflate_end:\n \tgit_inflate_end(&stream);\n cleanup:\n \tmunmap(map, mapsize);\n@@ -2623,12 +2615,8 @@ int read_loose_object(const char *path,\n \t\tgoto out;\n \t}\n \n-\tswitch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n-\t\t\t\t    NULL)) {\n-\tcase ULHR_OK:\n-\t\tbreak;\n-\tcase ULHR_BAD:\n-\tcase ULHR_TOO_LONG:\n+\tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n+\t\t\t\tNULL) < 0) {\n \t\terror(_(\"unable to unpack header of %s\"), path);\n \t\tgoto out;\n \t}\ndiff --git a/streaming.c b/streaming.c\nindex fe54665d86e..4456a60348b 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -230,15 +230,10 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,\n \tst->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);\n \tif (!st->u.loose.mapped)\n \t\treturn -1;\n-\tswitch (unpack_loose_header(&st->z, st->u.loose.mapped,\n-\t\t\t\t    st->u.loose.mapsize, st->u.loose.hdr,\n-\t\t\t\t    sizeof(st->u.loose.hdr), NULL)) {\n-\tcase ULHR_OK:\n-\t\tbreak;\n-\tcase ULHR_BAD:\n-\tcase ULHR_TOO_LONG:\n+\tif (unpack_loose_header(&st->z, st->u.loose.mapped,\n+\t\t\t\tst->u.loose.mapsize, st->u.loose.hdr,\n+\t\t\t\tsizeof(st->u.loose.hdr), NULL) < 0)\n \t\tgoto error;\n-\t}\n \tif (parse_loose_header(st->u.loose.hdr, &oi) < 0 || *type < 0)\n \t\tgoto error;\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex dadf3b14583..d742697d3bf 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -536,12 +536,14 @@ do\n \t\t\tif test \"$arg2\" = \"-p\"\n \t\t\tthen\n \t\t\t\tcat >expect <<-EOF\n-\t\t\t\terror: header for $bogus_long_sha1 too long, exceeds 32 bytes\n+\t\t\t\terror: header too long, exceeds 32 bytes\n+\t\t\t\terror: unable to unpack $bogus_long_sha1 header\n \t\t\t\tfatal: Not a valid object name $bogus_long_sha1\n \t\t\t\tEOF\n \t\t\telse\n \t\t\t\tcat >expect <<-EOF\n-\t\t\t\terror: header for $bogus_long_sha1 too long, exceeds 32 bytes\n+\t\t\t\terror: header too long, exceeds 32 bytes\n+\t\t\t\terror: unable to unpack $bogus_long_sha1 header\n \t\t\t\tfatal: git cat-file: could not get object info\n \t\t\t\tEOF\n \t\t\tfi &&\n-- \n2.36.1.957.g2c13267e09b\n\n"},{"id":"455546","messageId":"xmqq35h4ltfz.fsf@gitster.g","threadId":"57776","inReplyTo":"RFC-patch-2.2-af0dfd017af-20220519T195055Z-avarab@gmail.com","subject":"Re: [RFC PATCH 2/2] object-file API: have unpack_loose_header() return \"int\" again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T04:27:28Z","receivedAt":"2022-05-20T04:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> +int unpack_loose_header(git_zstream *stream, unsigned char *map,\n> +\t\t\tunsigned long mapsize, void *buffer,\n> +\t\t\tunsigned long bufsiz, struct strbuf *header);\n\nSimpler is better as long as we don't make it too simple ;-)\n\n>  \tif (!header)\n> -\t\treturn ULHR_TOO_LONG;\n> +\t\treturn error(_(\"header too long, exceeds %d bytes\"),\n> +\t\t\t     MAX_HEADER_LEN);\n\nOK.\n\n> -\treturn ULHR_BAD;\n> +\treturn error(_(\"could not find end of corrupt long header\"));\n>  }\n\nOK.\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index dadf3b14583..d742697d3bf 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -536,12 +536,14 @@ do\n>  \t\t\tif test \"$arg2\" = \"-p\"\n>  \t\t\tthen\n>  \t\t\t\tcat >expect <<-EOF\n> -\t\t\t\terror: header for $bogus_long_sha1 too long, exceeds 32 bytes\n> +\t\t\t\terror: header too long, exceeds 32 bytes\n> +\t\t\t\terror: unable to unpack $bogus_long_sha1 header\n>  \t\t\t\tfatal: Not a valid object name $bogus_long_sha1\n>  \t\t\t\tEOF\n>  \t\t\telse\n>  \t\t\t\tcat >expect <<-EOF\n> -\t\t\t\terror: header for $bogus_long_sha1 too long, exceeds 32 bytes\n> +\t\t\t\terror: header too long, exceeds 32 bytes\n> +\t\t\t\terror: unable to unpack $bogus_long_sha1 header\n>  \t\t\t\tfatal: git cat-file: could not get object info\n>  \t\t\t\tEOF\n>  \t\t\tfi &&\n\nLooking good.\n\n"}]}