Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode

Martin von Zweigbergk [off-list ref] writes:

quoted

+       bases = get_merge_bases_many(derived, revs.nr, revs.commit, 0);
+ ...
+       if (revs.nr <= i)
+               return 1; /* not found */
+
+       printf("%s\n", sha1_to_hex(bases->item->object.sha1));
+       free_commit_list(bases);
+       return 0;

Should free_commit_list also be called in the two "return 1" cases
above? I suppose the process will exit soon after this, but that seems
to be true for all three cases.

You are right that the above is inconsistent. Because the code
intends to be called only once in the lifetime of the program,
it calls get_merge_bases_many() with cleanup set to 0, so in that
sense, not freeing them anywhere may make it even more clear that
this function expects to be shortly followed by a process exit.

quoted

diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index f80bba8..4f09db0 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh

@@ -230,4 +230,31 @@ test_expect_success 'criss-cross merge-base for octopus-step' '
        test_cmp expected.sorted actual.sorted
 '

+test_expect_success 'using reflog to find the fork point' '
+       git reset --hard &&
+       git checkout -b base $E &&
+
+       (
+               for count in 1 2 3 4 5
+               do
+                       git commit --allow-empty -m "Base commit #$count" &&
+                       git rev-parse HEAD >expect$count &&
+                       git checkout -B derived &&
+                       git commit --allow-empty -m "Derived #$count" &&
+                       git rev-parse HEAD >derived$count &&
+                       git checkout base || exit 1

I think this creates a history like

---E---B1--B2--B3--B4--B5 (base)
        \   \   \   \   \
         D1  D2  D3  D4  D5 (derived)

So I think the following test would pass even if you drop the
--fork-point. Did you mean to create a fan-shaped history by resetting
base to $E on every iteration above?

Just showing that I didn't think things deeply ;-)  I do agree that a
fan-shaped history would show what we want to do a lot better.

Thanks.

quoted

+                       git merge-base --fork-point base $(cat derived$count) >actual &&
+                       test_cmp expect$count actual || exit 1