{"thread":{"id":"57077","subject":"[PATCH V3 1/2] git-apply: add --quiet flag","startedAt":"2021-12-13T22:03:32Z","lastAt":"2021-12-17T22:41:22Z","messageCount":13,"participants":["Jerry Zhang","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"444038","messageId":"20211213220327.16042-1-jerry@skydio.com","threadId":"57077","inReplyTo":null,"subject":"[PATCH V3 1/2] git-apply: add --quiet flag","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-13T22:03:26Z","receivedAt":"2021-12-13T22:03:32Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Replace OPT_VERBOSE with OPT_VERBOSITY.\n\nThis adds a --quiet flag to \"git apply\" so\nthe user can turn down the verbosity.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV2->V3 \n- Reorganized into a patch series to capture\ndependencies between 2 git apply changes.\n\n Documentation/git-apply.txt | 7 ++++++-\n apply.c                     | 2 +-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex aa1ae56a25..a32ad64718 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -14,11 +14,11 @@ SYNOPSIS\n \t  [--allow-binary-replacement | --binary] [--reject] [-z]\n \t  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]\n \t  [--ignore-space-change | --ignore-whitespace]\n \t  [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t  [--exclude=<path>] [--include=<path>] [--directory=<root>]\n-\t  [--verbose] [--unsafe-paths] [<patch>...]\n+\t  [--verbose | --quiet] [--unsafe-paths] [<patch>...]\n \n DESCRIPTION\n -----------\n Reads the supplied diff output (i.e. \"a patch\") and applies it to files.\n When running from a subdirectory in a repository, patched paths\n@@ -226,10 +226,15 @@ behavior:\n --verbose::\n \tReport progress to stderr. By default, only a message about the\n \tcurrent patch being applied will be printed. This option will cause\n \tadditional information to be reported.\n \n+-q::\n+--quiet::\n+\tSuppress stderr output. Messages about patch status and progress\n+\twill not be printed.\n+\n --recount::\n \tDo not trust the line counts in the hunk headers, but infer them\n \tby inspecting the patch (e.g. after editing the patch without\n \tadjusting the hunk headers appropriately).\n \ndiff --git a/apply.c b/apply.c\nindex 64b226acd9..9f00f882a2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -5071,11 +5071,11 @@ int apply_parse_options(int argc, const char **argv,\n \t\t\tN_(\"don't expect at least one line of context\")),\n \t\tOPT_BOOL(0, \"reject\", &state->apply_with_reject,\n \t\t\tN_(\"leave the rejected hunks in corresponding *.rej files\")),\n \t\tOPT_BOOL(0, \"allow-overlap\", &state->allow_overlap,\n \t\t\tN_(\"allow overlapping hunks\")),\n-\t\tOPT__VERBOSE(&state->apply_verbosity, N_(\"be verbose\")),\n+\t\tOPT__VERBOSITY(&state->apply_verbosity),\n \t\tOPT_BIT(0, \"inaccurate-eof\", options,\n \t\t\tN_(\"tolerate incorrectly detected missing new-line at the end of file\"),\n \t\t\tAPPLY_OPT_INACCURATE_EOF),\n \t\tOPT_BIT(0, \"recount\", options,\n \t\t\tN_(\"do not trust the line counts in the hunk headers\"),\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"444039","messageId":"20211213220327.16042-2-jerry@skydio.com","threadId":"57077","inReplyTo":"20211213220327.16042-1-jerry@skydio.com","subject":"[PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-13T22:03:27Z","receivedAt":"2021-12-13T22:03:43Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Some users or scripts will pipe \"git diff\"\noutput to \"git apply\" when replaying diffs\nor commits. In these cases, they will rely\non the return value of \"git apply\" to know\nwhether the diff was applied successfully.\n\nHowever, for empty commits, \"git apply\" will\nfail. This complicates scripts since they\nhave to either buffer the diff and check\nits length, or run diff again with \"exit-code\",\nessentially doing the diff twice.\n\nAdd the \"--allow-empty\" flag to \"git apply\"\nwhich allows it to handle both empty diffs\nand empty commits created by \"git format-patch\n--always\" by doing nothing and returning 0.\n\nAdd tests for both with and without --allow-empty.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV4->V5\n- Reorganized into a patch series to capture\ndependencies between 2 git apply changes.\n\n Documentation/git-apply.txt |  6 +++++-\n apply.c                     |  8 ++++++--\n apply.h                     |  1 +\n t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----\n 4 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex a32ad64718..b6d77f4206 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -14,11 +14,11 @@ SYNOPSIS\n \t  [--allow-binary-replacement | --binary] [--reject] [-z]\n \t  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]\n \t  [--ignore-space-change | --ignore-whitespace]\n \t  [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t  [--exclude=<path>] [--include=<path>] [--directory=<root>]\n-\t  [--verbose | --quiet] [--unsafe-paths] [<patch>...]\n+\t  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]\n \n DESCRIPTION\n -----------\n Reads the supplied diff output (i.e. \"a patch\") and applies it to files.\n When running from a subdirectory in a repository, patched paths\n@@ -254,10 +254,14 @@ running `git apply --directory=modules/git-gui`.\n +\n When `git apply` is used as a \"better GNU patch\", the user can pass\n the `--unsafe-paths` option to override this safety check.  This option\n has no effect when `--index` or `--cached` is in use.\n \n+--allow-empty::\n+\tDon't return error for patches containing no diff. This includes\n+\tempty patches and patches with commit text only.\n+\n CONFIGURATION\n -------------\n \n apply.ignoreWhitespace::\n \tSet to 'change' if you want changes in whitespace to be ignored by default.\ndiff --git a/apply.c b/apply.c\nindex 9f00f882a2..afc1c6510e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4752,12 +4752,14 @@ static int apply_patch(struct apply_state *state,\n \t\t}\n \t\toffset += nr;\n \t}\n \n \tif (!list && !skipped_patch) {\n-\t\terror(_(\"unrecognized input\"));\n-\t\tres = -128;\n+\t\tif (!state->allow_empty) {\n+\t\t\terror(_(\"No valid patches in input (allow with \\\"--allow-empty\\\")\"));\n+\t\t\tres = -128;\n+\t\t}\n \t\tgoto end;\n \t}\n \n \tif (state->whitespace_error && (state->ws_error_action == die_on_ws_error))\n \t\tstate->apply = 0;\n@@ -5081,10 +5083,12 @@ int apply_parse_options(int argc, const char **argv,\n \t\t\tN_(\"do not trust the line counts in the hunk headers\"),\n \t\t\tAPPLY_OPT_RECOUNT),\n \t\tOPT_CALLBACK(0, \"directory\", state, N_(\"root\"),\n \t\t\tN_(\"prepend <root> to all filenames\"),\n \t\t\tapply_option_parse_directory),\n+\t\tOPT_BOOL(0, \"allow-empty\", &state->allow_empty,\n+\t\t\tN_(\"don't return error for empty patches\")),\n \t\tOPT_END()\n \t};\n \n \treturn parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);\n }\ndiff --git a/apply.h b/apply.h\nindex da3d95fa50..16202da160 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -64,10 +64,11 @@ struct apply_state {\n \tint apply_with_reject;\n \tint no_add;\n \tint threeway;\n \tint unidiff_zero;\n \tint unsafe_paths;\n+\tint allow_empty;\n \n \t/* Other non boolean parameters */\n \tstruct repository *repo;\n \tconst char *index_file;\n \tenum apply_verbosity apply_verbosity;\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex ceb6a79fe0..949e284d14 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -7,10 +7,12 @@ test_description='apply empty'\n test_expect_success setup '\n \t>empty &&\n \tgit add empty &&\n \ttest_tick &&\n \tgit commit -m initial &&\n+\tgit commit --allow-empty -m \"empty commit\" &&\n+\tgit format-patch --always HEAD~ >empty.patch &&\n \tfor i in a b c d e\n \tdo\n \t\techo $i\n \tdone >empty &&\n \tcat empty >expect &&\n@@ -23,34 +25,46 @@ test_expect_success setup '\n \t>empty &&\n \tgit update-index --refresh\n '\n \n test_expect_success 'apply empty' '\n-\tgit reset --hard &&\n \trm -f missing &&\n+\ttest_when_finished \"git reset --hard\" &&\n \tgit apply patch0 &&\n \ttest_cmp expect empty\n '\n \n+test_expect_success 'apply empty patch fails' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\ttest_must_fail git apply empty.patch &&\n+\ttest_must_fail git apply - </dev/null\n+'\n+\n+test_expect_success 'apply with --allow-empty succeeds' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit apply --allow-empty empty.patch &&\n+\tgit apply --allow-empty - </dev/null\n+'\n+\n test_expect_success 'apply --index empty' '\n-\tgit reset --hard &&\n \trm -f missing &&\n+\ttest_when_finished \"git reset --hard\" &&\n \tgit apply --index patch0 &&\n \ttest_cmp expect empty &&\n \tgit diff --exit-code\n '\n \n test_expect_success 'apply create' '\n-\tgit reset --hard &&\n \trm -f missing &&\n+\ttest_when_finished \"git reset --hard\" &&\n \tgit apply patch1 &&\n \ttest_cmp expect missing\n '\n \n test_expect_success 'apply --index create' '\n-\tgit reset --hard &&\n \trm -f missing &&\n+\ttest_when_finished \"git reset --hard\" &&\n \tgit apply --index patch1 &&\n \ttest_cmp expect missing &&\n \tgit diff --exit-code\n '\n \n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"444043","messageId":"xmqqmtl49lzd.fsf@gitster.g","threadId":"57077","inReplyTo":"20211213220327.16042-1-jerry@skydio.com","subject":"Re: [PATCH V3 1/2] git-apply: add --quiet flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-13T22:30:14Z","receivedAt":"2021-12-13T22:30:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> Replace OPT_VERBOSE with OPT_VERBOSITY.\n>\n> This adds a --quiet flag to \"git apply\" so the user can turn down\n> the verbosity.\n>\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> ---\n> V2->V3 \n> - Reorganized into a patch series to capture\n> dependencies between 2 git apply changes.\n>\n>  Documentation/git-apply.txt | 7 ++++++-\n>  apply.c                     | 2 +-\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> index aa1ae56a25..a32ad64718 100644\n> --- a/Documentation/git-apply.txt\n> +++ b/Documentation/git-apply.txt\n> @@ -14,11 +14,11 @@ SYNOPSIS\n>  \t  [--allow-binary-replacement | --binary] [--reject] [-z]\n>  \t  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]\n>  \t  [--ignore-space-change | --ignore-whitespace]\n>  \t  [--whitespace=(nowarn|warn|fix|error|error-all)]\n>  \t  [--exclude=<path>] [--include=<path>] [--directory=<root>]\n> -\t  [--verbose] [--unsafe-paths] [<patch>...]\n> +\t  [--verbose | --quiet] [--unsafe-paths] [<patch>...]\n>  \n>  DESCRIPTION\n>  -----------\n>  Reads the supplied diff output (i.e. \"a patch\") and applies it to files.\n>  When running from a subdirectory in a repository, patched paths\n> @@ -226,10 +226,15 @@ behavior:\n>  --verbose::\n>  \tReport progress to stderr. By default, only a message about the\n>  \tcurrent patch being applied will be printed. This option will cause\n>  \tadditional information to be reported.\n>  \n> +-q::\n> +--quiet::\n> +\tSuppress stderr output. Messages about patch status and progress\n> +\twill not be printed.\n> +\n>  --recount::\n>  \tDo not trust the line counts in the hunk headers, but infer them\n>  \tby inspecting the patch (e.g. after editing the patch without\n>  \tadjusting the hunk headers appropriately).\n>  \n> diff --git a/apply.c b/apply.c\n> index 64b226acd9..9f00f882a2 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -5071,11 +5071,11 @@ int apply_parse_options(int argc, const char **argv,\n>  \t\t\tN_(\"don't expect at least one line of context\")),\n>  \t\tOPT_BOOL(0, \"reject\", &state->apply_with_reject,\n>  \t\t\tN_(\"leave the rejected hunks in corresponding *.rej files\")),\n>  \t\tOPT_BOOL(0, \"allow-overlap\", &state->allow_overlap,\n>  \t\t\tN_(\"allow overlapping hunks\")),\n> -\t\tOPT__VERBOSE(&state->apply_verbosity, N_(\"be verbose\")),\n> +\t\tOPT__VERBOSITY(&state->apply_verbosity),\n>  \t\tOPT_BIT(0, \"inaccurate-eof\", options,\n>  \t\t\tN_(\"tolerate incorrectly detected missing new-line at the end of file\"),\n>  \t\t\tAPPLY_OPT_INACCURATE_EOF),\n>  \t\tOPT_BIT(0, \"recount\", options,\n>  \t\t\tN_(\"do not trust the line counts in the hunk headers\"),\n\nIt is a bit surprising that this is the only change that is needed.\n\napply.h has\n\n    enum apply_verbosity {\n            verbosity_silent = -1,\n            verbosity_normal = 0,\n            verbosity_verbose = 1\n    };\n\nbut OPT__VERBOSITY() cna take more than one --verbose or --quiet to\ntune the verbosity level beyond the 1 and -1 limit.\n\nI looked at the output from\n\n    $ git grep -A3 -e '\\([.]\\|->\\)apply_verbosity'\n\nand made sure that there is no exact comparison with\nverbosity_silent or verbosity_verbose, which means we are OK.\n\nIt would have saved time to have a note in the proposed log message\nthat the author already audited and found that the existing code is\nready to accept verbosity values outside the \"enum apply_verbosity\"\nrange.\n\nThanks, will queue.\n"},{"id":"444194","messageId":"xmqqee6dz5s9.fsf@gitster.g","threadId":"57077","inReplyTo":"20211213220327.16042-2-jerry@skydio.com","subject":"Re: [PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-16T01:40:06Z","receivedAt":"2021-12-16T01:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n>  t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----\n>  4 files changed, 30 insertions(+), 7 deletions(-)\n> ...\n> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\n> index ceb6a79fe0..949e284d14 100755\n> --- a/t/t4126-apply-empty.sh\n> +++ b/t/t4126-apply-empty.sh\n> @@ -7,10 +7,12 @@ test_description='apply empty'\n>  test_expect_success setup '\n>  \t>empty &&\n>  \tgit add empty &&\n>  \ttest_tick &&\n>  \tgit commit -m initial &&\n> +\tgit commit --allow-empty -m \"empty commit\" &&\n> +\tgit format-patch --always HEAD~ >empty.patch &&\n>  \tfor i in a b c d e\n\nWhen merged with anything that has ab/mark-leak-free-tests-even-more\ntopic, this will start breaking the tests, as it is my understanding\nthat \"git log\" family hasn't been audited and converted for leak\nsanitizer.\n\nThis is sort of water under the bridge, as the other topic is\nalready in 'master', but come to think of it, the strategy we used\nwith TEST_PASSES_SANITIZE_LEAK variable was misguided.  \n\nIf the git subcommands a single test script uses were only the\nsubcommands that the test script wants to test, the approach to\ndefault to \"this subcommand has not been made leak sanitizer clean\",\nand then to add TEST_PASSES mark as we sanitize the subcommand makes\nperfect sense, but most test scripts need to run git subcommands\nthat are *not* the focus of the test---they run them only to prepare\nthe scene in which the subcommands being tested are excersized.  In\nsuch a situation (which is exactly what happens here), marking that\n\"right now, all the tested subcommands and also all the subcommands\nthat happen to be exercised to prepare fixture are clean\" would\nforce us to flip-flop with \"now we use a subcommand we didn't use in\nthis script before to prepare the scene, and it is not yet sanitizer\nclean, so we need to unmark it\", which is not quite ideal, but is\nmuch better than forcing the contributor who is *not* working on making\nthese subcommands leak-sanitizer-clean to worry about such a breakage.\n\nI am tempted to drop the \"TEST_PASSES\" bit from this script for now,\nbut I have to say that the \"mark leak-free tests\" topic took us in\nan awkward place.  We probably want to do something a bit more fine\ngrained about it.\n\nThanks.\n"},{"id":"444274","messageId":"xmqqtuf86t7z.fsf_-_@gitster.g","threadId":"57077","inReplyTo":"xmqqee6dz5s9.fsf@gitster.g","subject":"[PATCH] t4204 is not sanitizer clean at all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-16T23:11:12Z","receivedAt":"2021-12-16T23:11:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier we marked that this patch-id test is leak-sanitizer clean,\nbut if we read the test script carefully, it is merely because we\nhave too many invocations of commands in the \"git log\" family on the\nupstream side of the pipe, hiding breakages from them.\n\nSplit the pipeline so that breakages from these commands can be\ncaught (not limited to aborts due to leak-sanitizer) and unmark\nthe script as not passing the test with leak-sanitizer in effect.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n A quick grep tells me that tests 3302, 3303, 3305, 4020 and 6236\n all use \"git log\" and still are marked as passing tests with\n leak-sanitizer in effect.  I've taken a deep look at none of them,\n but I suspect they share the same kind of breakage.\n\n t/t4204-patch-id.sh | 29 +++++++++++++++++------------\n 1 file changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git i/t/t4204-patch-id.sh w/t/t4204-patch-id.sh\nindex e78d8097f3..80f4a65b28 100755\n--- i/t/t4204-patch-id.sh\n+++ w/t/t4204-patch-id.sh\n@@ -5,7 +5,6 @@ test_description='git patch-id'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n@@ -28,7 +27,8 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'patch-id output is well-formed' '\n-\tgit log -p -1 | git patch-id >output &&\n+\tgit log -p -1 >log.output &&\n+\tgit patch-id <log.output >output &&\n \tgrep \"^$OID_REGEX $(git rev-parse HEAD)$\" output\n '\n \n@@ -36,8 +36,8 @@ test_expect_success 'patch-id output is well-formed' '\n calc_patch_id () {\n \tpatch_name=\"$1\"\n \tshift\n-\tgit patch-id \"$@\" |\n-\tsed \"s/ .*//\" >patch-id_\"$patch_name\" &&\n+\tgit patch-id \"$@\" >patch-id.output &&\n+\tsed \"s/ .*//\" patch-id.output >patch-id_\"$patch_name\" &&\n \ttest_line_count -gt 0 patch-id_\"$patch_name\"\n }\n \n@@ -46,7 +46,8 @@ get_top_diff () {\n }\n \n get_patch_id () {\n-\tget_top_diff \"$1\" | calc_patch_id \"$@\"\n+\tget_top_diff \"$1\" >top-diff.output &&\n+\tcalc_patch_id <top-diff.output \"$@\"\n }\n \n test_expect_success 'patch-id detects equality' '\n@@ -64,16 +65,18 @@ test_expect_success 'patch-id detects inequality' '\n test_expect_success 'patch-id supports git-format-patch output' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-\tgit format-patch -1 --stdout | calc_patch_id same &&\n+\tgit format-patch -1 --stdout >format-patch.output &&\n+\tcalc_patch_id same <format-patch.output &&\n \ttest_cmp patch-id_main patch-id_same &&\n-\tset $(git format-patch -1 --stdout | git patch-id) &&\n+\tset $(git patch-id <format-patch.output) &&\n \ttest \"$2\" = $(git rev-parse HEAD)\n '\n \n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-\tgit format-patch -1 --stdout | sed \"s/ \\$//\" | calc_patch_id same &&\n+\tgit format-patch -1 --stdout >format-patch.output &&\n+\tsed \"s/ \\$//\" format-patch.output | calc_patch_id same &&\n \ttest_cmp patch-id_main patch-id_same\n '\n \n@@ -92,10 +95,11 @@ test_patch_id_file_order () {\n \tshift\n \tname=\"order-${1}-$relevant\"\n \tshift\n-\tget_top_diff \"main\" | calc_patch_id \"$name\" \"$@\" &&\n+\tget_top_diff \"main\" >top-diff.output &&\n+\tcalc_patch_id <top-diff.output \"$name\" \"$@\" &&\n \tgit checkout same &&\n-\tgit format-patch -1 --stdout -O foo-then-bar |\n-\t\tcalc_patch_id \"ordered-$name\" \"$@\" &&\n+\tgit format-patch -1 --stdout -O foo-then-bar >format-patch.output &&\n+\tcalc_patch_id <format-patch.output \"ordered-$name\" \"$@\" &&\n \tcmp_patch_id $relevant \"$name\" \"ordered-$name\"\n \n }\n@@ -143,7 +147,8 @@ test_expect_success '--stable overrides patchid.stable = false' '\n test_expect_success 'patch-id supports git-format-patch MIME output' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-\tgit format-patch -1 --attach --stdout | calc_patch_id same &&\n+\tgit format-patch -1 --attach --stdout >format-patch.output &&\n+\tcalc_patch_id <format-patch.output same &&\n \ttest_cmp patch-id_main patch-id_same\n '\n \n"},{"id":"444282","messageId":"xmqqpmpw6s0p.fsf_-_@gitster.g","threadId":"57077","inReplyTo":"xmqqee6dz5s9.fsf@gitster.g","subject":"[PATCH] format-patch: mark rev_info with UNLEAK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-16T23:37:10Z","receivedAt":"2021-12-16T23:37:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The comand uses a single instance of rev_info on stack, makes a\nsingle revision traversal and exit.  Mark the resources held by the\nrev_info structure with UNLEAK().\n\nWe do not do this at lower level in revision.c or cmd_log_walk(), as\na new caller of the revision traversal API can make unbounded number\nof rev_info during a single run, and UNLEAK() would not a be\nsuitable mechanism to deal with such a caller.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n    Junio C Hamano <gitster@pobox.com> writes:\n\n    > Jerry Zhang <jerry@skydio.com> writes:\n    >\n    >>  t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----\n    >>  4 files changed, 30 insertions(+), 7 deletions(-)\n    >> ...\n    >> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\n    >> index ceb6a79fe0..949e284d14 100755\n    >> --- a/t/t4126-apply-empty.sh\n    >> +++ b/t/t4126-apply-empty.sh\n    >> @@ -7,10 +7,12 @@ test_description='apply empty'\n    >>  test_expect_success setup '\n    >>  \t>empty &&\n    >>  \tgit add empty &&\n    >>  \ttest_tick &&\n    >>  \tgit commit -m initial &&\n    >> +\tgit commit --allow-empty -m \"empty commit\" &&\n    >> +\tgit format-patch --always HEAD~ >empty.patch &&\n    >>  \tfor i in a b c d e\n    >\n    > When merged with anything that has ab/mark-leak-free-tests-even-more\n    > topic, this will start breaking the tests, as it is my understanding\n    > that \"git log\" family hasn't been audited and converted for leak\n    > sanitizer.\n    > ...\n    > I am tempted to drop the \"TEST_PASSES\" bit from this script for now,\n    > but I have to say that the \"mark leak-free tests\" topic took us in\n    > an awkward place.  We probably want to do something a bit more fine\n    > grained about it.\n\n    Luckily, this test script is small enough that format-patch is the\n    only new offender, it seems, and with the attached patch I plan to\n    queue on a separate topic merged, it seem it no longer upsets the\n    sanitizer.\n\n builtin/log.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7..a7bca8353b 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2241,6 +2241,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&rdiff1);\n \tstrbuf_release(&rdiff2);\n \tstrbuf_release(&rdiff_title);\n+\tUNLEAK(rev);\n \treturn 0;\n }\n \n-- \n2.34.1-472-g213ab46be7\n"},{"id":"444324","messageId":"211217.861r2bal75.gmgdl@evledraar.gmail.com","threadId":"57077","inReplyTo":"xmqqtuf86t7z.fsf_-_@gitster.g","subject":"Re: [PATCH] t4204 is not sanitizer clean at all","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:39:04Z","receivedAt":"2021-12-17T04:50:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 16 2021, Junio C Hamano wrote:\n\n> Earlier we marked that this patch-id test is leak-sanitizer clean,\n> but if we read the test script carefully, it is merely because we\n> have too many invocations of commands in the \"git log\" family on the\n> upstream side of the pipe, hiding breakages from them.\n>\n> Split the pipeline so that breakages from these commands can be\n> caught (not limited to aborts due to leak-sanitizer) and unmark\n> the script as not passing the test with leak-sanitizer in effect.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  A quick grep tells me that tests 3302, 3303, 3305, 4020 and 6236\n>  all use \"git log\" and still are marked as passing tests with\n>  leak-sanitizer in effect.  I've taken a deep look at none of them,\n>  but I suspect they share the same kind of breakage.\n\nThis change looks good to me.\n\nFWIW this is not a mistake on my part, but something I'm perfectly aware\nof. I don't consider it to be \"brekage\".\n\nWe have plenty of place in the test suite where we hide exit codes on\nthe LHS of a pipe, or where we call a function that doesn't &&-chain its\ngit invocations.\n\nIn those cases we can and usually will \"succeed\" under LSAN, because it\nallows the program to emit its full output, and will abort() at the very\nend.\n\nI have an unsubmitted logging mode (using LSAN_OPTIONS=log_path=<path>)\nwhere I log every one of these to test-results/*, there's a lot more of\nthese.\n\nBut in the meantime I think the best way forward is to gradually mark\nthe tests that pass with LSAN as passing, to ensure that we at least\ndon't have regressions in the meantime. Before this we'd at least check\nthe \"git checkout\" etc. for leaks.\n\nIf I made fixing all broken &&-chains or git on the LHS of a pipe a\nprerequisite for marking as passing under under LSAN I'd end up with\nsomething that's approximately the size of [1] and more (i.e. Eric's\nupcoming patches to do that).\n\nI don't see why we'd consider perfect the enemy of the good in these\ncases. Yes we won't catch the successful exit of every single git\ninvocation, but our tests aren't doing that now, LSAN or not. But until\nthat's fixed we'll at least catch some, which helps our overall memory\nleak regression coverage.\n\nMore importantly it makes it a lot easier to reason about future memory\nleak patches, as we'll be able to get to a 1=1 mapping of tests that\npass, and those that are marked being known to pass. I'm using that\nlocally to fake-fail those that start passing unexpectedly that aren't\non the list, which then helps to inform the addition of \"this test now\npasses with no leaks\".\n\n1. https://lore.kernel.org/git/20211213063059.19424-1-sunshine@sunshineco.com/\n"},{"id":"444331","messageId":"211217.86wnk395bz.gmgdl@evledraar.gmail.com","threadId":"57077","inReplyTo":"xmqqee6dz5s9.fsf@gitster.g","subject":"Re: [PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:51:59Z","receivedAt":"2021-12-17T05:19:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 15 2021, Junio C Hamano wrote:\n\n> Jerry Zhang <jerry@skydio.com> writes:\n>\n>>  t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----\n>>  4 files changed, 30 insertions(+), 7 deletions(-)\n>> ...\n>> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\n>> index ceb6a79fe0..949e284d14 100755\n>> --- a/t/t4126-apply-empty.sh\n>> +++ b/t/t4126-apply-empty.sh\n>> @@ -7,10 +7,12 @@ test_description='apply empty'\n>>  test_expect_success setup '\n>>  \t>empty &&\n>>  \tgit add empty &&\n>>  \ttest_tick &&\n>>  \tgit commit -m initial &&\n>> +\tgit commit --allow-empty -m \"empty commit\" &&\n>> +\tgit format-patch --always HEAD~ >empty.patch &&\n>>  \tfor i in a b c d e\n>\n> When merged with anything that has ab/mark-leak-free-tests-even-more\n> topic, this will start breaking the tests, as it is my understanding\n> that \"git log\" family hasn't been audited and converted for leak\n> sanitizer.\n>\n> This is sort of water under the bridge, as the other topic is\n> already in 'master', but come to think of it, the strategy we used\n> with TEST_PASSES_SANITIZE_LEAK variable was misguided.  \n>\n> If the git subcommands a single test script uses were only the\n> subcommands that the test script wants to test, the approach to\n> default to \"this subcommand has not been made leak sanitizer clean\",\n> and then to add TEST_PASSES mark as we sanitize the subcommand makes\n> perfect sense, but most test scripts need to run git subcommands\n> that are *not* the focus of the test---they run them only to prepare\n> the scene in which the subcommands being tested are excersized.  In\n> such a situation (which is exactly what happens here), marking that\n> \"right now, all the tested subcommands and also all the subcommands\n> that happen to be exercised to prepare fixture are clean\" would\n> force us to flip-flop with \"now we use a subcommand we didn't use in\n> this script before to prepare the scene, and it is not yet sanitizer\n> clean, so we need to unmark it\", which is not quite ideal, but is\n> much better than forcing the contributor who is *not* working on making\n> these subcommands leak-sanitizer-clean to worry about such a breakage.\n>\n> I am tempted to drop the \"TEST_PASSES\" bit from this script for now,\n> but I have to say that the \"mark leak-free tests\" topic took us in\n> an awkward place.  We probably want to do something a bit more fine\n> grained about it.\n\nI don't see how us not having a 1=1 mapping between say a \"mktag.sh\"\ntest script and that script *only* running \"git mktag\" makes the\napproach with SANITIZE=leak misguided.\n\nYou can, FWIW, mark things in a more gradual manner than un-marking the\nscript entirely. There's the SANITIZE_LEAK prerequisite for individual\n\"test_expect_success\".\n\nYes it's painful that topics in-flight have this happen to them, but\nthat pain will mostly go away one the \"big leaks\" are solved,\ni.e. checkout/commit/log etc.\n\nI have all those patches, but they've been held up by the pace these\nchanges have been getting integrated at.\n\nE.g. f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more',\n2021-12-15) just hit master, but that series has been on-list since the\n31st of October, and was picked up & noted in What's Cooking on the 2nd\nof November[3]. The only changes in it are adding the same\n\"TEST_PASSES_SANITIZE_LEAK=true\" marking to 104 test scripts.\n\nPart of that delay is the release that happened mid-November, but even\naccounting for that I wish we could find ways to make this go\nfaster.\n\nI.e. I understand that a general change to git.git might take this time,\nbut in this case really all the proof we should need is \"does CI\npass?\". So I don't see why we couldn't make this go a bit faster.\n\nSimilarly for things that add new free()'s we can (unless the code is\ntricky, which is usually obvious, i.e. adding highly conditional free)\ncount on libc/SANITIZE=[leak|address] to validate that the memory\nmanagement is OK.\n\nAnyway, I had hoped to submit the \"struct rev_info\" freeing sometime\nsoon, depending on how you'd queue up other prerequisites for it.\n\nBut with an UNLEAK() for it in-flight I'll delay it even more, since it\nwill directly conflict both textually & semantically with the changes to\nfix the same memory leak.\n\nSo as painful as other parts of this are I'd really like it if we could\navoid taking one step back and two steps forward each step of the way by\nplastering over things with UNLEAK(). See [4] for earlier discussion on\nthat.\n\n1. https://lore.kernel.org/git/?q=ab%2Fmark-leak-free-tests-even-more\n2. https://lore.kernel.org/git/cover-00.15-00000000000-20211030T221945Z-avarab@gmail.com/\n3. https://lore.kernel.org/git/xmqqy267851e.fsf@gitster.g/\n4. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/\n\n\n\n"},{"id":"444391","messageId":"xmqqr1ab2c0v.fsf@gitster.g","threadId":"57077","inReplyTo":"211217.86wnk395bz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-17T20:48:32Z","receivedAt":"2021-12-17T20:48: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> I don't see how us not having a 1=1 mapping between say a \"mktag.sh\"\n> test script and that script *only* running \"git mktag\" makes the\n> approach with SANITIZE=leak misguided.\n\nSorry, if I was not clear.  SANITIZE=leak tests are perfectly fine.\n\nWhat I consider misguided is to mark each test script with\nTEST_PASSES marker.\n\nWe will *NOT* have \"this script uses 'git tag' to check it, and\nnothing else\", ever.  It is simply impossible to test the behaviour\nof a single command, as we need other git commands to prepare the\nscene for the command being tested to work in, and other git\ncommands to observe the outcome.  We'd run \"git commit\" to prepare a\ncommit before we can 'git tag' to tag it, and 'git verify-tag' to\nsee if the signature is good.\n\nAnd the approach to say \"at this point in time, sanitize test passes\nbecause all the git command we happen to use in this test script are\nsanitize-clean\" is misguided, when done way too early.  Because it\nis not just a statement about the state of the file at one point in\ntime, but it is a declaration that anybody touches the file is now\nresponsible for new leaks that triggers in that test script,\nregardless of how the leaks come.\n\nSurely, I am sympathetic to the intent.  If you are updating \"git\nfrotz\" that is sanitizer-clean, and if you write a new test in a\ntest script that happens to be sanitizer-clean, if you introduced a\nnew leak to \"git frotz\", you would appreciate if the CI notices it\nand blocks you.\n\nBut it is not the only way to get blockoed by CI.  You may need to\nuse another git subcommand that is known not to be sanitizer-clean\nyet to set things up or validate the result of the new feature you\nadded to \"git frotz\", and use of these commands will be caught as a\n\"new leak in the script file\", even if your change to \"git frotz\"\nintroduced no new leaks.\n\nThe only time we can sensibly do the \"now these are leak-free, and\nwe will catch and yell at you when you add a new leak\" is when we\nknow _all_ git commands are sanitize clean; then _any_ future change\nto _any_ git command that introduce a new leak can be caught.  Doing\nso before that is way too early, especially when only 230 among 940\nscripts can be marked as clean (and there are ones that are\nincorrectly marked as clean, too).  There is a very high chance for\nany of these 230 that are marked as \"clean\" to need to use a git\ncommand that is not yet sanitizer ready to set up the scene or\nvalidate the result, when a change is made to a command that is\nalready clean and is the target of the test.\n\n> You can, FWIW, mark things in a more gradual manner than un-marking the\n> script entirely. There's the SANITIZE_LEAK prerequisite for individual\n> \"test_expect_success\".\n\nThat will *NOT* work for the setup step, and you know it.\n\nWhat would have been nicer was a more gradual and finer-grained\napproach.  If we ignore feasibility for a moment, the ideal would be\nto have a central catalog of commands that are already sanitizer\nclean, so that test framework, when running a git command that is\nknown to be leaky, would disable sanitizer to avoid triggering its\noutput and non-zero exit, while enabling the sanitizer to catch any\nnew leaks in a git command that was known and declared to be\nleak-free (which was the reason why it was placed on that catalog).\n\nIf we had something like that, we wouldn't be having this discussion\non this thread, which is about improving the \"git apply\" command,\nnot about plugging known leaks in \"format-patch\" command.  \"apply\"\nwould have been on the \"clean\" list, and the \"format-patch\" whose\nuse is introduced to the \"setup\" step in this series is known to be\nunclean.\n\nMerging down the \"mark more of them as sanitizer-clean\" topic at\nf346fcb6 (Merge branch 'ab/mark-leak-free-tests-even-more',\n2021-12-15) was a mistake.  It was way too early, but unfortunately\nreverting and waiting would not help all that much, as the tests the\npatches in that topic touch will be updated while it is waiting, and\nthe point of the topic is to take a snapshot and to declare that all\nthe git commands it happens to use are leak-free, at least in the way\nthey are used in the script.\n\nHaving said that, what would be the next step to help developers to\navoid introducing new leaks while yelling at them for existing leaks\nthey did not introduce and not forbidding them to use git subccommands\nwith existing leaks in their tests?\n\nI would prefer an approach that does not force the project to make\nit the highest priority to plug leaks over everything else.\n\nHopefully, this time I was clear enough?\n\nThanks.\n"},{"id":"444392","messageId":"xmqqk0g32c06.fsf@gitster.g","threadId":"57077","inReplyTo":"211217.861r2bal75.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t4204 is not sanitizer clean at all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-17T20:48:57Z","receivedAt":"2021-12-17T20:49:03Z","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> This change looks good to me.\n>\n> FWIW this is not a mistake on my part, but something I'm perfectly aware\n> of. I don't consider it to be \"brekage\".\n>\n> We have plenty of place in the test suite where we hide exit codes on\n> the LHS of a pipe, or where we call a function that doesn't &&-chain its\n> git invocations.\n>\n> In those cases we can and usually will \"succeed\" under LSAN, because it\n> allows the program to emit its full output, and will abort() at the very\n> end.\n\nBut pipes do not hide ONLY deaths by sanitizer.  And by relying on\nthe presence of pipe hiding deaths of git tools to mark the script\nsanitizer-clean, the TEST_PASSES_SANITIZE_LEAK=true line adds an\nunnecessary road-block for those who are cleaning up the \"git whose\ncrash are hidden by being on the left hand side of the pipe\"\npattern.\n\nI do not know what to call it if not \"breakage\".\n\n"},{"id":"444395","messageId":"211217.86a6gyyihr.gmgdl@evledraar.gmail.com","threadId":"57077","inReplyTo":"xmqqk0g32c06.fsf@gitster.g","subject":"Re: [PATCH] t4204 is not sanitizer clean at all","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T22:23:00Z","receivedAt":"2021-12-17T22:27:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 17 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> This change looks good to me.\n>>\n>> FWIW this is not a mistake on my part, but something I'm perfectly aware\n>> of. I don't consider it to be \"brekage\".\n>>\n>> We have plenty of place in the test suite where we hide exit codes on\n>> the LHS of a pipe, or where we call a function that doesn't &&-chain its\n>> git invocations.\n>>\n>> In those cases we can and usually will \"succeed\" under LSAN, because it\n>> allows the program to emit its full output, and will abort() at the very\n>> end.\n>\n> But pipes do not hide ONLY deaths by sanitizer.  And by relying on\n> the presence of pipe hiding deaths of git tools to mark the script\n> sanitizer-clean, the TEST_PASSES_SANITIZE_LEAK=true line adds an\n> unnecessary road-block for those who are cleaning up the \"git whose\n> crash are hidden by being on the left hand side of the pipe\"\n> pattern.\n>\n> I do not know what to call it if not \"breakage\".\n\nYes it's broken as far as the test is concerned. I meant as far as\n\"GIT_TEST_PASSING_SANITIZE_LEAK\" goes I consider it somewhere between\n\"meh\" and \"don't care yet\".\n\nI.e. these are pretty irrelevant for finding leaks, as we've got a huge\ndeluge of them elsewhere. At some point we might have a last few stray\nmemory leaks in git hidden by such patterns, but we're very far away\nfrom that.\n\nSometimes fixing those is trivial as in 3247919a758 (commit-graph tests:\nfix error-hiding graph_git_two_modes() helper, 2021-10-15), and\nsometimes we'll find that the test was broken all along in some other\nsubtle way, as in the a046aa38ca9 (commit-graph tests: fix another\ngraph_git_two_modes() helper, 2021-10-15) follow-up.\n\nBut as to the \"roadblock\" I don't mind the\nTEST_PASSES_SANITIZE_LEAK=true being removed from the script at the\nslightest sign of trouble. Nobody should have to shift gears and chase\ndown some memory leak in \"git log\" just because they needed it for their\ntest setup.\n\nAnd I'd very much prefer that to UNLEAK() just to avoid that\nTEST_PASSES_SANITIZE_LEAK=true removal, because it makes fixing the leak\nitself harder as far as what topic to target, re-adding\nTEST_PASSES_SANITIZE_LEAK=true once it's fixed etc. goes.\n"},{"id":"444396","messageId":"xmqqee6a3lso.fsf@gitster.g","threadId":"57077","inReplyTo":"xmqqr1ab2c0v.fsf@gitster.g","subject":"Re: [PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-17T22:32:07Z","receivedAt":"2021-12-17T22:32:10Z","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> Surely, I am sympathetic to the intent.  If you are updating \"git\n> frotz\" that is sanitizer-clean, and if you write a new test in a\n> test script that happens to be sanitizer-clean, if you introduced a\n> new leak to \"git frotz\", you would appreciate if the CI notices it\n> and blocks you.\n> ...\n> The only time we can sensibly do the \"now these are leak-free, and\n> we will catch and yell at you when you add a new leak\" is when we\n> know _all_ git commands are sanitize clean...\n\nThere is another scenario where the TEST_PASSES_SANITIZE_LEAK=true\nmay make sense, actually.  If we declare that from the time we\ncommit to the approach, until we can mark all the test scripts with\nthe mark, we will put it the sole priority to squash any and all\nleaks, without doing anything else so that we can finish it the\nsoonest possible.\n\nThen it is probably OK to start at 230 and cover all 940 as fast as\nwe can.  Because we are effectively closing the tree for anything\nbut plug-leak changes and adding TEST_PASSES_SANITIZE_LEAK=true line\nto more tests, we wouldn't have to worry about introducing new leaks\nto existing tests that are marked as already clean---because of the\ntree closure, they are more likely to stay clean.  t4126 wouldn't\nhave gained a new use of format-patch to break it.\n\nBut of course, such an approach is not feasible in this project,\nwhere people do not work in lock-step.  That leads to the question I\nasked at the end of my previous message.\n\n> Having said that, what would be the next step to help developers to\n> avoid introducing new leaks while yelling at them for existing leaks\n> they did not introduce and not forbidding them to use git subccommands\n> with existing leaks in their tests?\n>\n> I would prefer an approach that does not force the project to make\n> it the highest priority to plug leaks over everything else.\n\nThanks.\n"},{"id":"444397","messageId":"211217.865yrmyhv8.gmgdl@evledraar.gmail.com","threadId":"57077","inReplyTo":"xmqqr1ab2c0v.fsf@gitster.g","subject":"Re: [PATCH V5 2/2] git-apply: add --allow-empty flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T22:28:04Z","receivedAt":"2021-12-17T22:41:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 17 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> I don't see how us not having a 1=1 mapping between say a \"mktag.sh\"\n>> test script and that script *only* running \"git mktag\" makes the\n>> approach with SANITIZE=leak misguided.\n>\n> Sorry, if I was not clear.  SANITIZE=leak tests are perfectly fine.\n>\n> What I consider misguided is to mark each test script with\n> TEST_PASSES marker.\n>\n> We will *NOT* have \"this script uses 'git tag' to check it, and\n> nothing else\", ever.  It is simply impossible to test the behaviour\n> of a single command, as we need other git commands to prepare the\n> scene for the command being tested to work in, and other git\n> commands to observe the outcome.  We'd run \"git commit\" to prepare a\n> commit before we can 'git tag' to tag it, and 'git verify-tag' to\n> see if the signature is good.\n>\n> And the approach to say \"at this point in time, sanitize test passes\n> because all the git command we happen to use in this test script are\n> sanitize-clean\" is misguided, when done way too early.  Because it\n> is not just a statement about the state of the file at one point in\n> time, but it is a declaration that anybody touches the file is now\n> responsible for new leaks that triggers in that test script,\n> regardless of how the leaks come.\n\nAs I just noted in the side-thread I think we should just recommend\nremoving the \"TEST_PASSES_SANITIZE_LEAK=true\" at the sligtest hint of\ntrouble:\nhttps://lore.kernel.org/git/211217.86a6gyyihr.gmgdl@evledraar.gmail.com/\n\nI think that should mostly address this as a problem in practice.\n\n> Surely, I am sympathetic to the intent.  If you are updating \"git\n> frotz\" that is sanitizer-clean, and if you write a new test in a\n> test script that happens to be sanitizer-clean, if you introduced a\n> new leak to \"git frotz\", you would appreciate if the CI notices it\n> and blocks you.\n>\n> But it is not the only way to get blockoed by CI.  You may need to\n> use another git subcommand that is known not to be sanitizer-clean\n> yet to set things up or validate the result of the new feature you\n> added to \"git frotz\", and use of these commands will be caught as a\n> \"new leak in the script file\", even if your change to \"git frotz\"\n> introduced no new leaks.\n>\n> The only time we can sensibly do the \"now these are leak-free, and\n> we will catch and yell at you when you add a new leak\" is when we\n> know _all_ git commands are sanitize clean; then _any_ future change\n> to _any_ git command that introduce a new leak can be caught.  Doing\n> so before that is way too early, especially when only 230 among 940\n> scripts can be marked as clean (and there are ones that are\n> incorrectly marked as clean, too).  There is a very high chance for\n> any of these 230 that are marked as \"clean\" to need to use a git\n> command that is not yet sanitizer ready to set up the scene or\n> validate the result, when a change is made to a command that is\n> already clean and is the target of the test.\n>\n>> You can, FWIW, mark things in a more gradual manner than un-marking the\n>> script entirely. There's the SANITIZE_LEAK prerequisite for individual\n>> \"test_expect_success\".\n>\n> That will *NOT* work for the setup step, and you know it.\n\nYes. I mean sometimes you can us that, or \"test_done\" early under that\nmode, or just un-mark the whole script by removing the\n\"TEST_PASSES_SANITIZE_LEAK=true\" line.\n\n> What would have been nicer was a more gradual and finer-grained\n> approach.  If we ignore feasibility for a moment, the ideal would be\n> to have a central catalog of commands that are already sanitizer\n> clean, so that test framework, when running a git command that is\n> known to be leaky, would disable sanitizer to avoid triggering its\n> output and non-zero exit, while enabling the sanitizer to catch any\n> new leaks in a git command that was known and declared to be\n> leak-free (which was the reason why it was placed on that catalog).\n>\n> If we had something like that, we wouldn't be having this discussion\n> on this thread, which is about improving the \"git apply\" command,\n> not about plugging known leaks in \"format-patch\" command.  \"apply\"\n> would have been on the \"clean\" list, and the \"format-patch\" whose\n> use is introduced to the \"setup\" step in this series is known to be\n> unclean.\n\nFWIW if we're going back to the drawing board a more viable way of doing\nthis (which I do locally) is to instrument LSAN to log normalized stack\ntraces, and then whitelist or blacklist certain stacktrace start/end\nmarkers.\n\nThat allows you to whitelist something like a cmd_apply, but importantly\ndoesn't limit you to just that, and you can at some point whitelist\nsetup_revisions, declare that no leak should be attributed downstream of\nmailmap.c etc.\n\n> Merging down the \"mark more of them as sanitizer-clean\" topic at\n> f346fcb6 (Merge branch 'ab/mark-leak-free-tests-even-more',\n> 2021-12-15) was a mistake.  It was way too early, but unfortunately\n> reverting and waiting would not help all that much, as the tests the\n> patches in that topic touch will be updated while it is waiting, and\n> the point of the topic is to take a snapshot and to declare that all\n> the git commands it happens to use are leak-free, at least in the way\n> they are used in the script.\n\n[...]\n\n> Having said that, what would be the next step to help developers to\n> avoid introducing new leaks while yelling at them for existing leaks\n> they did not introduce and not forbidding them to use git subccommands\n> with existing leaks in their tests?\n>\n> I would prefer an approach that does not force the project to make\n> it the highest priority to plug leaks over everything else.\n>\n> Hopefully, this time I was clear enough?\n\nYes, as noted in the interim we shouldn't hesitate to just remove\nindividual \"TEST_PASSES_SANITIZE_LEAK=true\".\n\nAs for the best way forward I think this will all be much less painful\nonce some of the \"big\" leaks are fixed. I.e. revision.c, \"git commit\"\netc.\n\nI've had those changes locally for a while now, but it's been slow going\nwith the whole submission/cooking etc. cycle. I didn't expect it to be\npainful for this long, sorry.\n"}]}