Re: [PATCH v2 1/2] Regression test for https://github.com/gitgitgadget/git/pull/1577

[Junio C Hamano <gitster@pobox.com>]

"Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH v2 1/2] Regression test for https://github.com/gitgitgadget/git/pull/1577

We try to come up with titles that are helpful to readers when seen
in "git shortlog --since=6.months --no-merges", and the above does
not exactly it.

> From: Kevin Backhouse <kevinbackhouse@github.com>
>
> Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>
> ---
>  t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100755 t/t9904-merge-leak.sh
>
> diff --git a/t/t9904-merge-leak.sh b/t/t9904-merge-leak.sh
> new file mode 100755
> index 00000000000..09a4474fd73
> --- /dev/null
> +++ b/t/t9904-merge-leak.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +#
> +
> +test_description='regression test for memory leak in git merge'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./lib-bash.sh
> +
> +# test-lib.sh disables LeakSanitizer by default, but we want it enabled
> +# for this test
> +ASAN_OPTIONS=
> +export ASAN_OPTIONS

You do not want to do this.

We have CI jobs that run everybody under asan, ubsan etc., so it is
sufficient and much more preferrable to just add a reproduction
recipe to an _existing_ test that is about "git merge" (or if we
have "ort" specific one, "git merge -s ort").  Of course they would
not fail in jobs that do not enable asan, and that is expected and
perfectly OK.

Also, please check Documentation/CodingGuidelines for shell style
issues.

> +. "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"

Is this about testing prompts, or does the bug/leak appear only when
the prompt support is in use?  Could you explain why this is needed?

> +test_expect_success 'Merge fails due to local changes' '
> +	git init &&
> +	echo x > x.txt &&
> +	git add . &&
> +	git commit -m "WIP" &&
> +	git checkout -b dev &&
> +	echo y > x.txt &&
> +	git add . &&
> +	git commit -m "WIP" &&
> +	git checkout main &&
> +	echo z > x.txt &&
> +	git add . &&
> +	git commit -m "WIP" &&
> +	echo a > x.txt &&
> +	git add . &&
> +	echo "error: ''Your local changes to the following files would be overwritten by merge:''" >expected &&
> +	echo "  x.txt" >>expected &&
> +	echo "Merge with strategy ort failed." >>expected &&
> +	test_must_fail git merge -s ort dev 2>actual &&
> +	test_cmp expected actual
> +'

If this 1/2 adds a new test that is expected to fail without leak
fix, which has to wait until 2/2, it breaks the bisection.  In this
case, since it will be a simple addition to an existing test script,
having both tests and code changes in a single patch is the most
appropriate.

Thank you for working on this.


> +
> +test_done