* [PATCH V3 1/2] git-apply: add --quiet flag @ 2021-12-13 22:03 Jerry Zhang 2021-12-13 22:03 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Jerry Zhang 2021-12-13 22:30 ` [PATCH V3 1/2] git-apply: add --quiet flag Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Jerry Zhang @ 2021-12-13 22:03 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Replace OPT_VERBOSE with OPT_VERBOSITY. This adds a --quiet flag to "git apply" so the user can turn down the verbosity. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V2->V3 - Reorganized into a patch series to capture dependencies between 2 git apply changes. Documentation/git-apply.txt | 7 ++++++- apply.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index aa1ae56a25..a32ad64718 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt @@ -14,11 +14,11 @@ SYNOPSIS [--allow-binary-replacement | --binary] [--reject] [-z] [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached] [--ignore-space-change | --ignore-whitespace] [--whitespace=(nowarn|warn|fix|error|error-all)] [--exclude=<path>] [--include=<path>] [--directory=<root>] - [--verbose] [--unsafe-paths] [<patch>...] + [--verbose | --quiet] [--unsafe-paths] [<patch>...] DESCRIPTION ----------- Reads the supplied diff output (i.e. "a patch") and applies it to files. When running from a subdirectory in a repository, patched paths @@ -226,10 +226,15 @@ behavior: --verbose:: Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. +-q:: +--quiet:: + Suppress stderr output. Messages about patch status and progress + will not be printed. + --recount:: Do not trust the line counts in the hunk headers, but infer them by inspecting the patch (e.g. after editing the patch without adjusting the hunk headers appropriately). diff --git a/apply.c b/apply.c index 64b226acd9..9f00f882a2 100644 --- a/apply.c +++ b/apply.c @@ -5071,11 +5071,11 @@ int apply_parse_options(int argc, const char **argv, N_("don't expect at least one line of context")), OPT_BOOL(0, "reject", &state->apply_with_reject, N_("leave the rejected hunks in corresponding *.rej files")), OPT_BOOL(0, "allow-overlap", &state->allow_overlap, N_("allow overlapping hunks")), - OPT__VERBOSE(&state->apply_verbosity, N_("be verbose")), + OPT__VERBOSITY(&state->apply_verbosity), OPT_BIT(0, "inaccurate-eof", options, N_("tolerate incorrectly detected missing new-line at the end of file"), APPLY_OPT_INACCURATE_EOF), OPT_BIT(0, "recount", options, N_("do not trust the line counts in the hunk headers"), -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-13 22:03 [PATCH V3 1/2] git-apply: add --quiet flag Jerry Zhang @ 2021-12-13 22:03 ` Jerry Zhang 2021-12-16 1:40 ` Junio C Hamano 2021-12-13 22:30 ` [PATCH V3 1/2] git-apply: add --quiet flag Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Jerry Zhang @ 2021-12-13 22:03 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Some users or scripts will pipe "git diff" output to "git apply" when replaying diffs or commits. In these cases, they will rely on the return value of "git apply" to know whether the diff was applied successfully. However, for empty commits, "git apply" will fail. This complicates scripts since they have to either buffer the diff and check its length, or run diff again with "exit-code", essentially doing the diff twice. Add the "--allow-empty" flag to "git apply" which allows it to handle both empty diffs and empty commits created by "git format-patch --always" by doing nothing and returning 0. Add tests for both with and without --allow-empty. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V4->V5 - Reorganized into a patch series to capture dependencies between 2 git apply changes. Documentation/git-apply.txt | 6 +++++- apply.c | 8 ++++++-- apply.h | 1 + t/t4126-apply-empty.sh | 22 ++++++++++++++++++---- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index a32ad64718..b6d77f4206 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt @@ -14,11 +14,11 @@ SYNOPSIS [--allow-binary-replacement | --binary] [--reject] [-z] [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached] [--ignore-space-change | --ignore-whitespace] [--whitespace=(nowarn|warn|fix|error|error-all)] [--exclude=<path>] [--include=<path>] [--directory=<root>] - [--verbose | --quiet] [--unsafe-paths] [<patch>...] + [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...] DESCRIPTION ----------- Reads the supplied diff output (i.e. "a patch") and applies it to files. When running from a subdirectory in a repository, patched paths @@ -254,10 +254,14 @@ running `git apply --directory=modules/git-gui`. + When `git apply` is used as a "better GNU patch", the user can pass the `--unsafe-paths` option to override this safety check. This option has no effect when `--index` or `--cached` is in use. +--allow-empty:: + Don't return error for patches containing no diff. This includes + empty patches and patches with commit text only. + CONFIGURATION ------------- apply.ignoreWhitespace:: Set to 'change' if you want changes in whitespace to be ignored by default. diff --git a/apply.c b/apply.c index 9f00f882a2..afc1c6510e 100644 --- a/apply.c +++ b/apply.c @@ -4752,12 +4752,14 @@ static int apply_patch(struct apply_state *state, } offset += nr; } if (!list && !skipped_patch) { - error(_("unrecognized input")); - res = -128; + if (!state->allow_empty) { + error(_("No valid patches in input (allow with \"--allow-empty\")")); + res = -128; + } goto end; } if (state->whitespace_error && (state->ws_error_action == die_on_ws_error)) state->apply = 0; @@ -5081,10 +5083,12 @@ int apply_parse_options(int argc, const char **argv, N_("do not trust the line counts in the hunk headers"), APPLY_OPT_RECOUNT), OPT_CALLBACK(0, "directory", state, N_("root"), N_("prepend <root> to all filenames"), apply_option_parse_directory), + OPT_BOOL(0, "allow-empty", &state->allow_empty, + N_("don't return error for empty patches")), OPT_END() }; return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0); } diff --git a/apply.h b/apply.h index da3d95fa50..16202da160 100644 --- a/apply.h +++ b/apply.h @@ -64,10 +64,11 @@ struct apply_state { int apply_with_reject; int no_add; int threeway; int unidiff_zero; int unsafe_paths; + int allow_empty; /* Other non boolean parameters */ struct repository *repo; const char *index_file; enum apply_verbosity apply_verbosity; diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index ceb6a79fe0..949e284d14 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -7,10 +7,12 @@ test_description='apply empty' test_expect_success setup ' >empty && git add empty && test_tick && git commit -m initial && + git commit --allow-empty -m "empty commit" && + git format-patch --always HEAD~ >empty.patch && for i in a b c d e do echo $i done >empty && cat empty >expect && @@ -23,34 +25,46 @@ test_expect_success setup ' >empty && git update-index --refresh ' test_expect_success 'apply empty' ' - git reset --hard && rm -f missing && + test_when_finished "git reset --hard" && git apply patch0 && test_cmp expect empty ' +test_expect_success 'apply empty patch fails' ' + test_when_finished "git reset --hard" && + test_must_fail git apply empty.patch && + test_must_fail git apply - </dev/null +' + +test_expect_success 'apply with --allow-empty succeeds' ' + test_when_finished "git reset --hard" && + git apply --allow-empty empty.patch && + git apply --allow-empty - </dev/null +' + test_expect_success 'apply --index empty' ' - git reset --hard && rm -f missing && + test_when_finished "git reset --hard" && git apply --index patch0 && test_cmp expect empty && git diff --exit-code ' test_expect_success 'apply create' ' - git reset --hard && rm -f missing && + test_when_finished "git reset --hard" && git apply patch1 && test_cmp expect missing ' test_expect_success 'apply --index create' ' - git reset --hard && rm -f missing && + test_when_finished "git reset --hard" && git apply --index patch1 && test_cmp expect missing && git diff --exit-code ' -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-13 22:03 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Jerry Zhang @ 2021-12-16 1:40 ` Junio C Hamano 2021-12-16 23:11 ` [PATCH] t4204 is not sanitizer clean at all Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Junio C Hamano @ 2021-12-16 1:40 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jerry Zhang Jerry Zhang <jerry@skydio.com> writes: > t/t4126-apply-empty.sh | 22 ++++++++++++++++++---- > 4 files changed, 30 insertions(+), 7 deletions(-) > ... > diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh > index ceb6a79fe0..949e284d14 100755 > --- a/t/t4126-apply-empty.sh > +++ b/t/t4126-apply-empty.sh > @@ -7,10 +7,12 @@ test_description='apply empty' > test_expect_success setup ' > >empty && > git add empty && > test_tick && > git commit -m initial && > + git commit --allow-empty -m "empty commit" && > + git format-patch --always HEAD~ >empty.patch && > for i in a b c d e When merged with anything that has ab/mark-leak-free-tests-even-more topic, this will start breaking the tests, as it is my understanding that "git log" family hasn't been audited and converted for leak sanitizer. This is sort of water under the bridge, as the other topic is already in 'master', but come to think of it, the strategy we used with TEST_PASSES_SANITIZE_LEAK variable was misguided. If the git subcommands a single test script uses were only the subcommands that the test script wants to test, the approach to default to "this subcommand has not been made leak sanitizer clean", and then to add TEST_PASSES mark as we sanitize the subcommand makes perfect sense, but most test scripts need to run git subcommands that are *not* the focus of the test---they run them only to prepare the scene in which the subcommands being tested are excersized. In such a situation (which is exactly what happens here), marking that "right now, all the tested subcommands and also all the subcommands that happen to be exercised to prepare fixture are clean" would force us to flip-flop with "now we use a subcommand we didn't use in this script before to prepare the scene, and it is not yet sanitizer clean, so we need to unmark it", which is not quite ideal, but is much better than forcing the contributor who is *not* working on making these subcommands leak-sanitizer-clean to worry about such a breakage. I am tempted to drop the "TEST_PASSES" bit from this script for now, but I have to say that the "mark leak-free tests" topic took us in an awkward place. We probably want to do something a bit more fine grained about it. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] t4204 is not sanitizer clean at all 2021-12-16 1:40 ` Junio C Hamano @ 2021-12-16 23:11 ` Junio C Hamano 2021-12-17 4:39 ` Ævar Arnfjörð Bjarmason 2021-12-16 23:37 ` [PATCH] format-patch: mark rev_info with UNLEAK Junio C Hamano 2021-12-17 4:51 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2021-12-16 23:11 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git Earlier we marked that this patch-id test is leak-sanitizer clean, but if we read the test script carefully, it is merely because we have too many invocations of commands in the "git log" family on the upstream side of the pipe, hiding breakages from them. Split the pipeline so that breakages from these commands can be caught (not limited to aborts due to leak-sanitizer) and unmark the script as not passing the test with leak-sanitizer in effect. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- A quick grep tells me that tests 3302, 3303, 3305, 4020 and 6236 all use "git log" and still are marked as passing tests with leak-sanitizer in effect. I've taken a deep look at none of them, but I suspect they share the same kind of breakage. t/t4204-patch-id.sh | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git i/t/t4204-patch-id.sh w/t/t4204-patch-id.sh index e78d8097f3..80f4a65b28 100755 --- i/t/t4204-patch-id.sh +++ w/t/t4204-patch-id.sh @@ -5,7 +5,6 @@ test_description='git patch-id' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME -TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' @@ -28,7 +27,8 @@ test_expect_success 'setup' ' ' test_expect_success 'patch-id output is well-formed' ' - git log -p -1 | git patch-id >output && + git log -p -1 >log.output && + git patch-id <log.output >output && grep "^$OID_REGEX $(git rev-parse HEAD)$" output ' @@ -36,8 +36,8 @@ test_expect_success 'patch-id output is well-formed' ' calc_patch_id () { patch_name="$1" shift - git patch-id "$@" | - sed "s/ .*//" >patch-id_"$patch_name" && + git patch-id "$@" >patch-id.output && + sed "s/ .*//" patch-id.output >patch-id_"$patch_name" && test_line_count -gt 0 patch-id_"$patch_name" } @@ -46,7 +46,8 @@ get_top_diff () { } get_patch_id () { - get_top_diff "$1" | calc_patch_id "$@" + get_top_diff "$1" >top-diff.output && + calc_patch_id <top-diff.output "$@" } test_expect_success 'patch-id detects equality' ' @@ -64,16 +65,18 @@ test_expect_success 'patch-id detects inequality' ' test_expect_success 'patch-id supports git-format-patch output' ' get_patch_id main && git checkout same && - git format-patch -1 --stdout | calc_patch_id same && + git format-patch -1 --stdout >format-patch.output && + calc_patch_id same <format-patch.output && test_cmp patch-id_main patch-id_same && - set $(git format-patch -1 --stdout | git patch-id) && + set $(git patch-id <format-patch.output) && test "$2" = $(git rev-parse HEAD) ' test_expect_success 'whitespace is irrelevant in footer' ' get_patch_id main && git checkout same && - git format-patch -1 --stdout | sed "s/ \$//" | calc_patch_id same && + git format-patch -1 --stdout >format-patch.output && + sed "s/ \$//" format-patch.output | calc_patch_id same && test_cmp patch-id_main patch-id_same ' @@ -92,10 +95,11 @@ test_patch_id_file_order () { shift name="order-${1}-$relevant" shift - get_top_diff "main" | calc_patch_id "$name" "$@" && + get_top_diff "main" >top-diff.output && + calc_patch_id <top-diff.output "$name" "$@" && git checkout same && - git format-patch -1 --stdout -O foo-then-bar | - calc_patch_id "ordered-$name" "$@" && + git format-patch -1 --stdout -O foo-then-bar >format-patch.output && + calc_patch_id <format-patch.output "ordered-$name" "$@" && cmp_patch_id $relevant "$name" "ordered-$name" } @@ -143,7 +147,8 @@ test_expect_success '--stable overrides patchid.stable = false' ' test_expect_success 'patch-id supports git-format-patch MIME output' ' get_patch_id main && git checkout same && - git format-patch -1 --attach --stdout | calc_patch_id same && + git format-patch -1 --attach --stdout >format-patch.output && + calc_patch_id <format-patch.output same && test_cmp patch-id_main patch-id_same ' ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] t4204 is not sanitizer clean at all 2021-12-16 23:11 ` [PATCH] t4204 is not sanitizer clean at all Junio C Hamano @ 2021-12-17 4:39 ` Ævar Arnfjörð Bjarmason 2021-12-17 20:48 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-12-17 4:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Sunshine On Thu, Dec 16 2021, Junio C Hamano wrote: > Earlier we marked that this patch-id test is leak-sanitizer clean, > but if we read the test script carefully, it is merely because we > have too many invocations of commands in the "git log" family on the > upstream side of the pipe, hiding breakages from them. > > Split the pipeline so that breakages from these commands can be > caught (not limited to aborts due to leak-sanitizer) and unmark > the script as not passing the test with leak-sanitizer in effect. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > A quick grep tells me that tests 3302, 3303, 3305, 4020 and 6236 > all use "git log" and still are marked as passing tests with > leak-sanitizer in effect. I've taken a deep look at none of them, > but I suspect they share the same kind of breakage. This change looks good to me. FWIW this is not a mistake on my part, but something I'm perfectly aware of. I don't consider it to be "brekage". We have plenty of place in the test suite where we hide exit codes on the LHS of a pipe, or where we call a function that doesn't &&-chain its git invocations. In those cases we can and usually will "succeed" under LSAN, because it allows the program to emit its full output, and will abort() at the very end. I have an unsubmitted logging mode (using LSAN_OPTIONS=log_path=<path>) where I log every one of these to test-results/*, there's a lot more of these. But in the meantime I think the best way forward is to gradually mark the tests that pass with LSAN as passing, to ensure that we at least don't have regressions in the meantime. Before this we'd at least check the "git checkout" etc. for leaks. If I made fixing all broken &&-chains or git on the LHS of a pipe a prerequisite for marking as passing under under LSAN I'd end up with something that's approximately the size of [1] and more (i.e. Eric's upcoming patches to do that). I don't see why we'd consider perfect the enemy of the good in these cases. Yes we won't catch the successful exit of every single git invocation, but our tests aren't doing that now, LSAN or not. But until that's fixed we'll at least catch some, which helps our overall memory leak regression coverage. More importantly it makes it a lot easier to reason about future memory leak patches, as we'll be able to get to a 1=1 mapping of tests that pass, and those that are marked being known to pass. I'm using that locally to fake-fail those that start passing unexpectedly that aren't on the list, which then helps to inform the addition of "this test now passes with no leaks". 1. https://lore.kernel.org/git/20211213063059.19424-1-sunshine@sunshineco.com/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] t4204 is not sanitizer clean at all 2021-12-17 4:39 ` Ævar Arnfjörð Bjarmason @ 2021-12-17 20:48 ` Junio C Hamano 2021-12-17 22:23 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2021-12-17 20:48 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Sunshine Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This change looks good to me. > > FWIW this is not a mistake on my part, but something I'm perfectly aware > of. I don't consider it to be "brekage". > > We have plenty of place in the test suite where we hide exit codes on > the LHS of a pipe, or where we call a function that doesn't &&-chain its > git invocations. > > In those cases we can and usually will "succeed" under LSAN, because it > allows the program to emit its full output, and will abort() at the very > end. But pipes do not hide ONLY deaths by sanitizer. And by relying on the presence of pipe hiding deaths of git tools to mark the script sanitizer-clean, the TEST_PASSES_SANITIZE_LEAK=true line adds an unnecessary road-block for those who are cleaning up the "git whose crash are hidden by being on the left hand side of the pipe" pattern. I do not know what to call it if not "breakage". ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] t4204 is not sanitizer clean at all 2021-12-17 20:48 ` Junio C Hamano @ 2021-12-17 22:23 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-12-17 22:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Sunshine On Fri, Dec 17 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This change looks good to me. >> >> FWIW this is not a mistake on my part, but something I'm perfectly aware >> of. I don't consider it to be "brekage". >> >> We have plenty of place in the test suite where we hide exit codes on >> the LHS of a pipe, or where we call a function that doesn't &&-chain its >> git invocations. >> >> In those cases we can and usually will "succeed" under LSAN, because it >> allows the program to emit its full output, and will abort() at the very >> end. > > But pipes do not hide ONLY deaths by sanitizer. And by relying on > the presence of pipe hiding deaths of git tools to mark the script > sanitizer-clean, the TEST_PASSES_SANITIZE_LEAK=true line adds an > unnecessary road-block for those who are cleaning up the "git whose > crash are hidden by being on the left hand side of the pipe" > pattern. > > I do not know what to call it if not "breakage". Yes it's broken as far as the test is concerned. I meant as far as "GIT_TEST_PASSING_SANITIZE_LEAK" goes I consider it somewhere between "meh" and "don't care yet". I.e. these are pretty irrelevant for finding leaks, as we've got a huge deluge of them elsewhere. At some point we might have a last few stray memory leaks in git hidden by such patterns, but we're very far away from that. Sometimes fixing those is trivial as in 3247919a758 (commit-graph tests: fix error-hiding graph_git_two_modes() helper, 2021-10-15), and sometimes we'll find that the test was broken all along in some other subtle way, as in the a046aa38ca9 (commit-graph tests: fix another graph_git_two_modes() helper, 2021-10-15) follow-up. But as to the "roadblock" I don't mind the TEST_PASSES_SANITIZE_LEAK=true being removed from the script at the slightest sign of trouble. Nobody should have to shift gears and chase down some memory leak in "git log" just because they needed it for their test setup. And I'd very much prefer that to UNLEAK() just to avoid that TEST_PASSES_SANITIZE_LEAK=true removal, because it makes fixing the leak itself harder as far as what topic to target, re-adding TEST_PASSES_SANITIZE_LEAK=true once it's fixed etc. goes. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] format-patch: mark rev_info with UNLEAK 2021-12-16 1:40 ` Junio C Hamano 2021-12-16 23:11 ` [PATCH] t4204 is not sanitizer clean at all Junio C Hamano @ 2021-12-16 23:37 ` Junio C Hamano 2021-12-17 4:51 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-12-16 23:37 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jerry Zhang The comand uses a single instance of rev_info on stack, makes a single revision traversal and exit. Mark the resources held by the rev_info structure with UNLEAK(). We do not do this at lower level in revision.c or cmd_log_walk(), as a new caller of the revision traversal API can make unbounded number of rev_info during a single run, and UNLEAK() would not a be suitable mechanism to deal with such a caller. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Junio C Hamano <gitster@pobox.com> writes: > Jerry Zhang <jerry@skydio.com> writes: > >> t/t4126-apply-empty.sh | 22 ++++++++++++++++++---- >> 4 files changed, 30 insertions(+), 7 deletions(-) >> ... >> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh >> index ceb6a79fe0..949e284d14 100755 >> --- a/t/t4126-apply-empty.sh >> +++ b/t/t4126-apply-empty.sh >> @@ -7,10 +7,12 @@ test_description='apply empty' >> test_expect_success setup ' >> >empty && >> git add empty && >> test_tick && >> git commit -m initial && >> + git commit --allow-empty -m "empty commit" && >> + git format-patch --always HEAD~ >empty.patch && >> for i in a b c d e > > When merged with anything that has ab/mark-leak-free-tests-even-more > topic, this will start breaking the tests, as it is my understanding > that "git log" family hasn't been audited and converted for leak > sanitizer. > ... > I am tempted to drop the "TEST_PASSES" bit from this script for now, > but I have to say that the "mark leak-free tests" topic took us in > an awkward place. We probably want to do something a bit more fine > grained about it. Luckily, this test script is small enough that format-patch is the only new offender, it seems, and with the attached patch I plan to queue on a separate topic merged, it seem it no longer upsets the sanitizer. builtin/log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/log.c b/builtin/log.c index f75d87e8d7..a7bca8353b 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -2241,6 +2241,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) strbuf_release(&rdiff1); strbuf_release(&rdiff2); strbuf_release(&rdiff_title); + UNLEAK(rev); return 0; } -- 2.34.1-472-g213ab46be7 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-16 1:40 ` Junio C Hamano 2021-12-16 23:11 ` [PATCH] t4204 is not sanitizer clean at all Junio C Hamano 2021-12-16 23:37 ` [PATCH] format-patch: mark rev_info with UNLEAK Junio C Hamano @ 2021-12-17 4:51 ` Ævar Arnfjörð Bjarmason 2021-12-17 20:48 ` Junio C Hamano 2 siblings, 1 reply; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-12-17 4:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jerry Zhang On Wed, Dec 15 2021, Junio C Hamano wrote: > Jerry Zhang <jerry@skydio.com> writes: > >> t/t4126-apply-empty.sh | 22 ++++++++++++++++++---- >> 4 files changed, 30 insertions(+), 7 deletions(-) >> ... >> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh >> index ceb6a79fe0..949e284d14 100755 >> --- a/t/t4126-apply-empty.sh >> +++ b/t/t4126-apply-empty.sh >> @@ -7,10 +7,12 @@ test_description='apply empty' >> test_expect_success setup ' >> >empty && >> git add empty && >> test_tick && >> git commit -m initial && >> + git commit --allow-empty -m "empty commit" && >> + git format-patch --always HEAD~ >empty.patch && >> for i in a b c d e > > When merged with anything that has ab/mark-leak-free-tests-even-more > topic, this will start breaking the tests, as it is my understanding > that "git log" family hasn't been audited and converted for leak > sanitizer. > > This is sort of water under the bridge, as the other topic is > already in 'master', but come to think of it, the strategy we used > with TEST_PASSES_SANITIZE_LEAK variable was misguided. > > If the git subcommands a single test script uses were only the > subcommands that the test script wants to test, the approach to > default to "this subcommand has not been made leak sanitizer clean", > and then to add TEST_PASSES mark as we sanitize the subcommand makes > perfect sense, but most test scripts need to run git subcommands > that are *not* the focus of the test---they run them only to prepare > the scene in which the subcommands being tested are excersized. In > such a situation (which is exactly what happens here), marking that > "right now, all the tested subcommands and also all the subcommands > that happen to be exercised to prepare fixture are clean" would > force us to flip-flop with "now we use a subcommand we didn't use in > this script before to prepare the scene, and it is not yet sanitizer > clean, so we need to unmark it", which is not quite ideal, but is > much better than forcing the contributor who is *not* working on making > these subcommands leak-sanitizer-clean to worry about such a breakage. > > I am tempted to drop the "TEST_PASSES" bit from this script for now, > but I have to say that the "mark leak-free tests" topic took us in > an awkward place. We probably want to do something a bit more fine > grained about it. I don't see how us not having a 1=1 mapping between say a "mktag.sh" test script and that script *only* running "git mktag" makes the approach with SANITIZE=leak misguided. You can, FWIW, mark things in a more gradual manner than un-marking the script entirely. There's the SANITIZE_LEAK prerequisite for individual "test_expect_success". Yes it's painful that topics in-flight have this happen to them, but that pain will mostly go away one the "big leaks" are solved, i.e. checkout/commit/log etc. I have all those patches, but they've been held up by the pace these changes have been getting integrated at. E.g. f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more', 2021-12-15) just hit master, but that series has been on-list since the 31st of October, and was picked up & noted in What's Cooking on the 2nd of November[3]. The only changes in it are adding the same "TEST_PASSES_SANITIZE_LEAK=true" marking to 104 test scripts. Part of that delay is the release that happened mid-November, but even accounting for that I wish we could find ways to make this go faster. I.e. I understand that a general change to git.git might take this time, but in this case really all the proof we should need is "does CI pass?". So I don't see why we couldn't make this go a bit faster. Similarly for things that add new free()'s we can (unless the code is tricky, which is usually obvious, i.e. adding highly conditional free) count on libc/SANITIZE=[leak|address] to validate that the memory management is OK. Anyway, I had hoped to submit the "struct rev_info" freeing sometime soon, depending on how you'd queue up other prerequisites for it. But with an UNLEAK() for it in-flight I'll delay it even more, since it will directly conflict both textually & semantically with the changes to fix the same memory leak. So as painful as other parts of this are I'd really like it if we could avoid taking one step back and two steps forward each step of the way by plastering over things with UNLEAK(). See [4] for earlier discussion on that. 1. https://lore.kernel.org/git/?q=ab%2Fmark-leak-free-tests-even-more 2. https://lore.kernel.org/git/cover-00.15-00000000000-20211030T221945Z-avarab@gmail.com/ 3. https://lore.kernel.org/git/xmqqy267851e.fsf@gitster.g/ 4. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-17 4:51 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Ævar Arnfjörð Bjarmason @ 2021-12-17 20:48 ` Junio C Hamano 2021-12-17 22:28 ` Ævar Arnfjörð Bjarmason 2021-12-17 22:32 ` Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2021-12-17 20:48 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jerry Zhang Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I don't see how us not having a 1=1 mapping between say a "mktag.sh" > test script and that script *only* running "git mktag" makes the > approach with SANITIZE=leak misguided. Sorry, if I was not clear. SANITIZE=leak tests are perfectly fine. What I consider misguided is to mark each test script with TEST_PASSES marker. We will *NOT* have "this script uses 'git tag' to check it, and nothing else", ever. It is simply impossible to test the behaviour of a single command, as we need other git commands to prepare the scene for the command being tested to work in, and other git commands to observe the outcome. We'd run "git commit" to prepare a commit before we can 'git tag' to tag it, and 'git verify-tag' to see if the signature is good. And the approach to say "at this point in time, sanitize test passes because all the git command we happen to use in this test script are sanitize-clean" is misguided, when done way too early. Because it is not just a statement about the state of the file at one point in time, but it is a declaration that anybody touches the file is now responsible for new leaks that triggers in that test script, regardless of how the leaks come. Surely, I am sympathetic to the intent. If you are updating "git frotz" that is sanitizer-clean, and if you write a new test in a test script that happens to be sanitizer-clean, if you introduced a new leak to "git frotz", you would appreciate if the CI notices it and blocks you. But it is not the only way to get blockoed by CI. You may need to use another git subcommand that is known not to be sanitizer-clean yet to set things up or validate the result of the new feature you added to "git frotz", and use of these commands will be caught as a "new leak in the script file", even if your change to "git frotz" introduced no new leaks. The only time we can sensibly do the "now these are leak-free, and we will catch and yell at you when you add a new leak" is when we know _all_ git commands are sanitize clean; then _any_ future change to _any_ git command that introduce a new leak can be caught. Doing so before that is way too early, especially when only 230 among 940 scripts can be marked as clean (and there are ones that are incorrectly marked as clean, too). There is a very high chance for any of these 230 that are marked as "clean" to need to use a git command that is not yet sanitizer ready to set up the scene or validate the result, when a change is made to a command that is already clean and is the target of the test. > You can, FWIW, mark things in a more gradual manner than un-marking the > script entirely. There's the SANITIZE_LEAK prerequisite for individual > "test_expect_success". That will *NOT* work for the setup step, and you know it. What would have been nicer was a more gradual and finer-grained approach. If we ignore feasibility for a moment, the ideal would be to have a central catalog of commands that are already sanitizer clean, so that test framework, when running a git command that is known to be leaky, would disable sanitizer to avoid triggering its output and non-zero exit, while enabling the sanitizer to catch any new leaks in a git command that was known and declared to be leak-free (which was the reason why it was placed on that catalog). If we had something like that, we wouldn't be having this discussion on this thread, which is about improving the "git apply" command, not about plugging known leaks in "format-patch" command. "apply" would have been on the "clean" list, and the "format-patch" whose use is introduced to the "setup" step in this series is known to be unclean. Merging down the "mark more of them as sanitizer-clean" topic at f346fcb6 (Merge branch 'ab/mark-leak-free-tests-even-more', 2021-12-15) was a mistake. It was way too early, but unfortunately reverting and waiting would not help all that much, as the tests the patches in that topic touch will be updated while it is waiting, and the point of the topic is to take a snapshot and to declare that all the git commands it happens to use are leak-free, at least in the way they are used in the script. Having said that, what would be the next step to help developers to avoid introducing new leaks while yelling at them for existing leaks they did not introduce and not forbidding them to use git subccommands with existing leaks in their tests? I would prefer an approach that does not force the project to make it the highest priority to plug leaks over everything else. Hopefully, this time I was clear enough? Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-17 20:48 ` Junio C Hamano @ 2021-12-17 22:28 ` Ævar Arnfjörð Bjarmason 2021-12-17 22:32 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-12-17 22:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jerry Zhang On Fri, Dec 17 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I don't see how us not having a 1=1 mapping between say a "mktag.sh" >> test script and that script *only* running "git mktag" makes the >> approach with SANITIZE=leak misguided. > > Sorry, if I was not clear. SANITIZE=leak tests are perfectly fine. > > What I consider misguided is to mark each test script with > TEST_PASSES marker. > > We will *NOT* have "this script uses 'git tag' to check it, and > nothing else", ever. It is simply impossible to test the behaviour > of a single command, as we need other git commands to prepare the > scene for the command being tested to work in, and other git > commands to observe the outcome. We'd run "git commit" to prepare a > commit before we can 'git tag' to tag it, and 'git verify-tag' to > see if the signature is good. > > And the approach to say "at this point in time, sanitize test passes > because all the git command we happen to use in this test script are > sanitize-clean" is misguided, when done way too early. Because it > is not just a statement about the state of the file at one point in > time, but it is a declaration that anybody touches the file is now > responsible for new leaks that triggers in that test script, > regardless of how the leaks come. As I just noted in the side-thread I think we should just recommend removing the "TEST_PASSES_SANITIZE_LEAK=true" at the sligtest hint of trouble: https://lore.kernel.org/git/211217.86a6gyyihr.gmgdl@evledraar.gmail.com/ I think that should mostly address this as a problem in practice. > Surely, I am sympathetic to the intent. If you are updating "git > frotz" that is sanitizer-clean, and if you write a new test in a > test script that happens to be sanitizer-clean, if you introduced a > new leak to "git frotz", you would appreciate if the CI notices it > and blocks you. > > But it is not the only way to get blockoed by CI. You may need to > use another git subcommand that is known not to be sanitizer-clean > yet to set things up or validate the result of the new feature you > added to "git frotz", and use of these commands will be caught as a > "new leak in the script file", even if your change to "git frotz" > introduced no new leaks. > > The only time we can sensibly do the "now these are leak-free, and > we will catch and yell at you when you add a new leak" is when we > know _all_ git commands are sanitize clean; then _any_ future change > to _any_ git command that introduce a new leak can be caught. Doing > so before that is way too early, especially when only 230 among 940 > scripts can be marked as clean (and there are ones that are > incorrectly marked as clean, too). There is a very high chance for > any of these 230 that are marked as "clean" to need to use a git > command that is not yet sanitizer ready to set up the scene or > validate the result, when a change is made to a command that is > already clean and is the target of the test. > >> You can, FWIW, mark things in a more gradual manner than un-marking the >> script entirely. There's the SANITIZE_LEAK prerequisite for individual >> "test_expect_success". > > That will *NOT* work for the setup step, and you know it. Yes. I mean sometimes you can us that, or "test_done" early under that mode, or just un-mark the whole script by removing the "TEST_PASSES_SANITIZE_LEAK=true" line. > What would have been nicer was a more gradual and finer-grained > approach. If we ignore feasibility for a moment, the ideal would be > to have a central catalog of commands that are already sanitizer > clean, so that test framework, when running a git command that is > known to be leaky, would disable sanitizer to avoid triggering its > output and non-zero exit, while enabling the sanitizer to catch any > new leaks in a git command that was known and declared to be > leak-free (which was the reason why it was placed on that catalog). > > If we had something like that, we wouldn't be having this discussion > on this thread, which is about improving the "git apply" command, > not about plugging known leaks in "format-patch" command. "apply" > would have been on the "clean" list, and the "format-patch" whose > use is introduced to the "setup" step in this series is known to be > unclean. FWIW if we're going back to the drawing board a more viable way of doing this (which I do locally) is to instrument LSAN to log normalized stack traces, and then whitelist or blacklist certain stacktrace start/end markers. That allows you to whitelist something like a cmd_apply, but importantly doesn't limit you to just that, and you can at some point whitelist setup_revisions, declare that no leak should be attributed downstream of mailmap.c etc. > Merging down the "mark more of them as sanitizer-clean" topic at > f346fcb6 (Merge branch 'ab/mark-leak-free-tests-even-more', > 2021-12-15) was a mistake. It was way too early, but unfortunately > reverting and waiting would not help all that much, as the tests the > patches in that topic touch will be updated while it is waiting, and > the point of the topic is to take a snapshot and to declare that all > the git commands it happens to use are leak-free, at least in the way > they are used in the script. [...] > Having said that, what would be the next step to help developers to > avoid introducing new leaks while yelling at them for existing leaks > they did not introduce and not forbidding them to use git subccommands > with existing leaks in their tests? > > I would prefer an approach that does not force the project to make > it the highest priority to plug leaks over everything else. > > Hopefully, this time I was clear enough? Yes, as noted in the interim we shouldn't hesitate to just remove individual "TEST_PASSES_SANITIZE_LEAK=true". As for the best way forward I think this will all be much less painful once some of the "big" leaks are fixed. I.e. revision.c, "git commit" etc. I've had those changes locally for a while now, but it's been slow going with the whole submission/cooking etc. cycle. I didn't expect it to be painful for this long, sorry. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V5 2/2] git-apply: add --allow-empty flag 2021-12-17 20:48 ` Junio C Hamano 2021-12-17 22:28 ` Ævar Arnfjörð Bjarmason @ 2021-12-17 22:32 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-12-17 22:32 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Surely, I am sympathetic to the intent. If you are updating "git > frotz" that is sanitizer-clean, and if you write a new test in a > test script that happens to be sanitizer-clean, if you introduced a > new leak to "git frotz", you would appreciate if the CI notices it > and blocks you. > ... > The only time we can sensibly do the "now these are leak-free, and > we will catch and yell at you when you add a new leak" is when we > know _all_ git commands are sanitize clean... There is another scenario where the TEST_PASSES_SANITIZE_LEAK=true may make sense, actually. If we declare that from the time we commit to the approach, until we can mark all the test scripts with the mark, we will put it the sole priority to squash any and all leaks, without doing anything else so that we can finish it the soonest possible. Then it is probably OK to start at 230 and cover all 940 as fast as we can. Because we are effectively closing the tree for anything but plug-leak changes and adding TEST_PASSES_SANITIZE_LEAK=true line to more tests, we wouldn't have to worry about introducing new leaks to existing tests that are marked as already clean---because of the tree closure, they are more likely to stay clean. t4126 wouldn't have gained a new use of format-patch to break it. But of course, such an approach is not feasible in this project, where people do not work in lock-step. That leads to the question I asked at the end of my previous message. > Having said that, what would be the next step to help developers to > avoid introducing new leaks while yelling at them for existing leaks > they did not introduce and not forbidding them to use git subccommands > with existing leaks in their tests? > > I would prefer an approach that does not force the project to make > it the highest priority to plug leaks over everything else. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 1/2] git-apply: add --quiet flag 2021-12-13 22:03 [PATCH V3 1/2] git-apply: add --quiet flag Jerry Zhang 2021-12-13 22:03 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Jerry Zhang @ 2021-12-13 22:30 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-12-13 22:30 UTC (permalink / raw) To: Jerry Zhang; +Cc: git Jerry Zhang <jerry@skydio.com> writes: > Replace OPT_VERBOSE with OPT_VERBOSITY. > > This adds a --quiet flag to "git apply" so the user can turn down > the verbosity. > > Signed-off-by: Jerry Zhang <jerry@skydio.com> > --- > V2->V3 > - Reorganized into a patch series to capture > dependencies between 2 git apply changes. > > Documentation/git-apply.txt | 7 ++++++- > apply.c | 2 +- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > index aa1ae56a25..a32ad64718 100644 > --- a/Documentation/git-apply.txt > +++ b/Documentation/git-apply.txt > @@ -14,11 +14,11 @@ SYNOPSIS > [--allow-binary-replacement | --binary] [--reject] [-z] > [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached] > [--ignore-space-change | --ignore-whitespace] > [--whitespace=(nowarn|warn|fix|error|error-all)] > [--exclude=<path>] [--include=<path>] [--directory=<root>] > - [--verbose] [--unsafe-paths] [<patch>...] > + [--verbose | --quiet] [--unsafe-paths] [<patch>...] > > DESCRIPTION > ----------- > Reads the supplied diff output (i.e. "a patch") and applies it to files. > When running from a subdirectory in a repository, patched paths > @@ -226,10 +226,15 @@ behavior: > --verbose:: > Report progress to stderr. By default, only a message about the > current patch being applied will be printed. This option will cause > additional information to be reported. > > +-q:: > +--quiet:: > + Suppress stderr output. Messages about patch status and progress > + will not be printed. > + > --recount:: > Do not trust the line counts in the hunk headers, but infer them > by inspecting the patch (e.g. after editing the patch without > adjusting the hunk headers appropriately). > > diff --git a/apply.c b/apply.c > index 64b226acd9..9f00f882a2 100644 > --- a/apply.c > +++ b/apply.c > @@ -5071,11 +5071,11 @@ int apply_parse_options(int argc, const char **argv, > N_("don't expect at least one line of context")), > OPT_BOOL(0, "reject", &state->apply_with_reject, > N_("leave the rejected hunks in corresponding *.rej files")), > OPT_BOOL(0, "allow-overlap", &state->allow_overlap, > N_("allow overlapping hunks")), > - OPT__VERBOSE(&state->apply_verbosity, N_("be verbose")), > + OPT__VERBOSITY(&state->apply_verbosity), > OPT_BIT(0, "inaccurate-eof", options, > N_("tolerate incorrectly detected missing new-line at the end of file"), > APPLY_OPT_INACCURATE_EOF), > OPT_BIT(0, "recount", options, > N_("do not trust the line counts in the hunk headers"), It is a bit surprising that this is the only change that is needed. apply.h has enum apply_verbosity { verbosity_silent = -1, verbosity_normal = 0, verbosity_verbose = 1 }; but OPT__VERBOSITY() cna take more than one --verbose or --quiet to tune the verbosity level beyond the 1 and -1 limit. I looked at the output from $ git grep -A3 -e '\([.]\|->\)apply_verbosity' and made sure that there is no exact comparison with verbosity_silent or verbosity_verbose, which means we are OK. It would have saved time to have a note in the proposed log message that the author already audited and found that the existing code is ready to accept verbosity values outside the "enum apply_verbosity" range. Thanks, will queue. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-12-17 22:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-13 22:03 [PATCH V3 1/2] git-apply: add --quiet flag Jerry Zhang 2021-12-13 22:03 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Jerry Zhang 2021-12-16 1:40 ` Junio C Hamano 2021-12-16 23:11 ` [PATCH] t4204 is not sanitizer clean at all Junio C Hamano 2021-12-17 4:39 ` Ævar Arnfjörð Bjarmason 2021-12-17 20:48 ` Junio C Hamano 2021-12-17 22:23 ` Ævar Arnfjörð Bjarmason 2021-12-16 23:37 ` [PATCH] format-patch: mark rev_info with UNLEAK Junio C Hamano 2021-12-17 4:51 ` [PATCH V5 2/2] git-apply: add --allow-empty flag Ævar Arnfjörð Bjarmason 2021-12-17 20:48 ` Junio C Hamano 2021-12-17 22:28 ` Ævar Arnfjörð Bjarmason 2021-12-17 22:32 ` Junio C Hamano 2021-12-13 22:30 ` [PATCH V3 1/2] git-apply: add --quiet flag Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).