[PATCH V3 1/2] git-apply: add --quiet flag

[PATCH V3 1/2] git-apply: add --quiet flag

[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

[PATCH V5 2/2] git-apply: add --allow-empty flag

[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

Re: [PATCH V3 1/2] git-apply: add --quiet flag

[Junio C Hamano]

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.

Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

[Junio C Hamano]

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.

[PATCH] t4204 is not sanitizer clean at all

[Junio C Hamano]

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
 '
 

[PATCH] format-patch: mark rev_info with UNLEAK

[Junio C Hamano]

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

Re: [PATCH] t4204 is not sanitizer clean at all

[Ævar Arnfjörð Bjarmason]

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/

Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

[Ævar Arnfjörð Bjarmason]

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/

Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

[Junio C Hamano]

Æ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.

Re: [PATCH] t4204 is not sanitizer clean at all

[Junio C Hamano]

Æ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".

Re: [PATCH] t4204 is not sanitizer clean at all

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

[Junio C Hamano]

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.