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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2023, 15:11 UTC
Message-ID: <xmqqh6oocw5h.fsf@gitster.g>
In-Reply-To: <f940104a7814bd0b1e45f1230b2cb4b0be305699.1692886365.git.gitgitgadget@gmail.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.

Show 28 quoted lines

> 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?

Show 21 quoted lines

> +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