revision: fix memory leak in prepare_show_merge()

From: Lidong Yan <[email protected]>

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().

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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

Quoted 12 lines · 2025年6月4日 15:48，Patrick Steinhardt <[email protected]> 写道：

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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?

Quoted 2 lines

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.

Quoted 13 lines · lidongyan

Quoted 12 lines · 2025年6月4日 15:48，Patrick Steinhardt <[email protected]> 写道：

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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?

Quoted 8 lines

Quoted 2 lines

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

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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?

Quoted 10 lines · Patrick Steinhardt

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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

t/t7007-show.sh +0 -0

index d6cc69e0f2..99f4d0b963 100755
@@ -167,4 +167,19 @@ test_expect_success 'show --graph is forbidden' '
  test_must_fail git show --graph HEAD

'

t/t7007-show.sh +15 -0

+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

+ git rev-parse HEAD >.git/MERGE_HEAD &&

I could add this test case into my patch. Though I don’t understand

Quoted 1 line

+ git rev-parse HEAD >.git/MERGE_HEAD &&

If HEAD is equal to MERGE_HEAD. Would git show —merge still works as usual? How about something like this

t/t7007-show.sh +24 -0

index d6cc69e0f2..f693b6e24b 100755
@@ -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

Quoted 1 line · lidongyan

I could add this test case into my patch. Though I don’t understand

Quoted 1 line

Quoted 1 line

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

Quoted 36 lines

If HEAD is equal to MERGE_HEAD. Would git show —merge still works as usual? How about something like this

t/t7007-show.sh +0 -0

index d6cc69e0f2..f693b6e24b 100755
@@ -167,4 +167,28 @@ test_expect_success 'show --graph is forbidden' '
  test_must_fail git show --graph HEAD

'

t/t7007-show.sh +24 -0

+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

t/t7007-show.sh +19 -0

+    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