On Mon, Sep 30, 2019 at 11:58:49PM -0700, Elijah Newren wrote:
> In commit 743474cbfa8b ("merge-recursive: provide a better label for
> diff3 common ancestor", 2019-08-17), the label for the common ancestor
> was changed from always being
>
> "merged common ancestors"
>
> to instead be based on the number of merge bases:
>
> >=2: "merged common ancestors"
> 1: <abbreviated commit hash>
> 0: "<empty tree>"
>
> Unfortunately, this did not take into account that when we have a single
> merge base, that merge base could be fake or constructed. In such
> cases, this resulted in a label of "00000000". Of course, the previous
> label of "merged common ancestors" was also misleading for this case.
Yeah, I agree the original was not great, either, but the "0000000"
looked like a bug to me. Hey, at least we didn't segfault! :)
> Since we have an API that is explicitly about creating fake merge base
> commits in merge_recursive_generic(), we should provide a better label
> when using that API with one merge base. So, when
> merge_recursive_generic() is called with one merge base, set the label
> to:
>
> "constructed merge base"
That makes perfect sense to me. Thanks for the quick turnaround.
> Changes since v1:
> - We only had a problem if the number of fake merge bases was exactly
> one; update the patch to check for that and update the commit message
> accordingly.
That makes sense. We'd still want to say "merged common ancestors" even
if one of those ancestors was fake.
> merge-recursive.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
The patch itself looks good to me (though admittedly I'm not too
familiar with this area).
Maybe worth squashing in this test?
diff --git a/t/t6047-diff3-conflict-markers.sh
b/t/t6047-diff3-conflict-markers.sh
index 3fb68e0aae..860542aad0 100755
--- a/t/t6047-diff3-conflict-markers.sh
+++ b/t/t6047-diff3-conflict-markers.sh
@@ -186,4 +186,17 @@ test_expect_success 'check multiple merge bases' '
)
'
+test_expect_success 'rebase describes fake ancestor base' '
+ test_create_repo rebase &&
+ (
+ cd rebase &&
+ test_commit base file &&
+ test_commit master file &&
+ git checkout -b side HEAD^ &&
+ test_commit side file &&
+ test_must_fail git -c merge.conflictstyle=diff3 rebase master &&
+ grep "||||||| constructed merge base" file
+ )
+'
+
test_done
-Peff
