patchrevision: fix memory leak in prepare_show_merge()

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> In revision.c:prepare_show_merge(), we allocated an array in prune
> but forget to free it. Since parse_pathspec is not responsible to
> free prune, we should add `free(prune)` in the end of prepare_show_merge().

> 
> On Wed, Jun 04, 2025 at 03:08:56AM +0000, Lidong Yan via GitGitGadget wrote:
>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>> 
>> In revision.c:prepare_show_merge(), we allocated an array in prune
>> but forget to free it. Since parse_pathspec is not responsible to
>> free prune, we should add `free(prune)` in the end of prepare_show_merge().
> 
> That is a rather obvious memory leak indeed. Do you know why we never
> detected the leak in our CI? Is this code path not exercised at all by
> our tests?
> 
> Patrick
> 

> 2025年6月4日 15:48，Patrick Steinhardt <ps@pks.im> 写道：
> > 
> > On Wed, Jun 04, 2025 at 03:08:56AM +0000, Lidong Yan via GitGitGadget wrote:
> >> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> >> 
> >> In revision.c:prepare_show_merge(), we allocated an array in prune
> >> but forget to free it. Since parse_pathspec is not responsible to
> >> free prune, we should add `free(prune)` in the end of prepare_show_merge().
> > 
> > That is a rather obvious memory leak indeed. Do you know why we never
> > detected the leak in our CI? Is this code path not exercised at all by
> > our tests?
> > 
> > Patrick
> > 
> 
> I don’t know why CI test doesn’t cover this leak, but I am happy to add
> a prereq test for this case.
> 
> p.s. I also like to ask that it there anyway to run test locally? How do you
> developers normally run test without open an pull request.

> 
> On Wed, Jun 04, 2025 at 03:53:44PM +0800, lidongyan wrote:
>> 2025年6月4日 15:48，Patrick Steinhardt <ps@pks.im> 写道：
>>> 
>>> On Wed, Jun 04, 2025 at 03:08:56AM +0000, Lidong Yan via GitGitGadget wrote:
>>>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>>>> 
>>>> In revision.c:prepare_show_merge(), we allocated an array in prune
>>>> but forget to free it. Since parse_pathspec is not responsible to
>>>> free prune, we should add `free(prune)` in the end of prepare_show_merge().
>>> 
>>> That is a rather obvious memory leak indeed. Do you know why we never
>>> detected the leak in our CI? Is this code path not exercised at all by
>>> our tests?
>>> 
>>> Patrick
>>> 
>> 
>> I don’t know why CI test doesn’t cover this leak, but I am happy to add
>> a prereq test for this case.
>> 
>> p.s. I also like to ask that it there anyway to run test locally? How do you
>> developers normally run test without open an pull request.
> 
> Do you mean the test suite in general or leak tests in particular? In
> any case, you can of course run both of these locally. You can do so
> either by using Make:
> 
>    # Run tests.
>    $ make test
>    # Run tests with the leak checking enabled.
>    $ make test SANITIZE=leak
> 
> Or with Meson:
> 
>    # Create the build directory and execute tests.
>    $ meson setup build
>    $ meson test -C build
> 
>    # Create a second build directory, this time with leak checking
>    # enabled.
>    $ meson setup build-leaks -Db_sanitize=leak
>    $ meson test -C build-leaks
> 
> Patrick

> On Wed, Jun 04, 2025 at 03:08:56AM +0000, Lidong Yan via GitGitGadget wrote:
>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>> 
>> In revision.c:prepare_show_merge(), we allocated an array in prune
>> but forget to free it. Since parse_pathspec is not responsible to
>> free prune, we should add `free(prune)` in the end of prepare_show_merge().
>
> That is a rather obvious memory leak indeed. Do you know why we never
> detected the leak in our CI? Is this code path not exercised at all by
> our tests?

> 
> Patrick Steinhardt <ps@pks.im> writes:
> 
>> On Wed, Jun 04, 2025 at 03:08:56AM +0000, Lidong Yan via GitGitGadget wrote:
>>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>>> 
>>> In revision.c:prepare_show_merge(), we allocated an array in prune
>>> but forget to free it. Since parse_pathspec is not responsible to
>>> free prune, we should add `free(prune)` in the end of prepare_show_merge().
>> 
>> That is a rather obvious memory leak indeed. Do you know why we never
>> detected the leak in our CI? Is this code path not exercised at all by
>> our tests?
> 
> I think we have no "show --merge" test.  Something like this may be
> minimally sufficient.
> 
> t/t7007-show.sh | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git c/t/t7007-show.sh w/t/t7007-show.sh
> index d6cc69e0f2..99f4d0b963 100755
> --- c/t/t7007-show.sh
> +++ w/t/t7007-show.sh
> @@ -167,4 +167,19 @@ test_expect_success 'show --graph is forbidden' '
>   test_must_fail git show --graph HEAD
> '
> 
> +test_expect_success 'unmerged index' '
> + git reset --hard &&
> + git commit --allow-empty -m initial &&
> + git rev-parse HEAD >.git/MERGE_HEAD &&
> + blob1=$(echo hello | git hash-object -w --stdin) &&
> + blob2=$(echo goodbye | git hash-object -w --stdin) &&
> + blob3=$(echo world | git hash-object -w --stdin) &&
> + git update-index --add --index-info <<-EOF &&
> + 100644 $blob1 1 conflicting
> + 100644 $blob2 2 conflicting
> + 100755 $blob3 3 conflicting
> + EOF
> + git show --merge HEAD
> +'
> +
> test_done
> 

> If HEAD is equal to MERGE_HEAD. Would git show —merge still
> works as usual? How about something like this
>
> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
> index d6cc69e0f2..f693b6e24b 100755
> --- a/t/t7007-show.sh
> +++ b/t/t7007-show.sh
> @@ -167,4 +167,28 @@ test_expect_success 'show --graph is forbidden' '
>    test_must_fail git show --graph HEAD
>  '
>  
> +test_expect_success 'unmerged index' '
> +       git reset --hard &&
> +
> +       git switch -C base &&
> +       echo "base" > conflicting &&
> +       git add conflicting &&
> +       git commit -m "base" &&
> +
> +       git branch hello &&
> +       git branch goodbye &&
> +
> +       git switch hello &&
> +       echo "hello" > conflicting &&
> +       git commit -am "hello" &&
> +
> +       git switch goodbye &&
> +       echo "goodbye" > conflicting &&
> +       git commit -am "goodbye" &&
> +
> +       git switch hello &&
> +       test_must_fail git merge goodbye &&
> +       git show --merge HEAD
> +'
> +
>  test_done

> 
> lidongyan <502024330056@smail.nju.edu.cn> writes:
> 
>> I could add this test case into my patch. Though I don’t understand
>>> + git rev-parse HEAD >.git/MERGE_HEAD &&
> 
> That was not about telling "show --merge" to work on any meaningful
> data and to produce any useful output.  I knew the step to prepare
> for "show --merge" was leaky, so I gave a very minimum that can tickle
> that codepath.  I wasn't of course proud of the direct manipulation
> of the filesystem (as recent "git update-ref MERGE_HEAD HEAD" would
> not even allow us to do this, sheesh, not very convenient).

> 
> If you came up with a sequence that produces a situation to use the
> "git show --merge" command in a more realistic way, like below, that
> is wonderful.
> 
>> If HEAD is equal to MERGE_HEAD. Would git show —merge still
>> works as usual? How about something like this
>> 
>> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
>> index d6cc69e0f2..f693b6e24b 100755
>> --- a/t/t7007-show.sh
>> +++ b/t/t7007-show.sh
>> @@ -167,4 +167,28 @@ test_expect_success 'show --graph is forbidden' '
>>   test_must_fail git show --graph HEAD
>> '
>> 
>> +test_expect_success 'unmerged index' '
>> +       git reset --hard &&
>> +
>> +       git switch -C base &&
>> +       echo "base" > conflicting &&
>> +       git add conflicting &&
>> +       git commit -m "base" &&
>> +
>> +       git branch hello &&
>> +       git branch goodbye &&
>> +
>> +       git switch hello &&
>> +       echo "hello" > conflicting &&
>> +       git commit -am "hello" &&
>> +
>> +       git switch goodbye &&
>> +       echo "goodbye" > conflicting &&
>> +       git commit -am "goodbye" &&
>> +
>> +       git switch hello &&
>> +       test_must_fail git merge goodbye &&
>> +       git show --merge HEAD
>> +'
>> +
>> test_done
> 

> +    git add conflicting &&
> +    git commit -m "base" &&
> +
> +    git branch hello &&
> +    git branch goodbye &&
> +
> +    git switch hello &&
> +    echo "hello" > conflicting &&
> +    git commit -am "hello" &&
> +
> +    git switch goodbye &&
> +    echo "goodbye" > conflicting &&
> +    git commit -am "goodbye" &&
> +
> +    git switch hello &&
> +    test_must_fail git merge goodbye &&
> +    git show --merge HEAD
> +'
> +
>  test_done
>
> base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67