Re: [PATCH v2] merge-recursive: fix the diff3 common ancestor label for virtual commits
- From
Jeff King <peff@peff.net>
- Date
- Oct 1, 2019, 14:49 UTC
- Message-ID
- <20191001144940.GH10875@sigill.intra.peff.net>
- In-Reply-To
- <20191001065849.29491-1-newren@gmail.com>
On Mon, Sep 30, 2019 at 11:58:49PM -0700, Elijah Newren wrote:
Show 16 quoted lines
> 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! :)
Show 7 quoted lines
> 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