git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jan 9, 2018, 18:27 UTC
Message-ID
<CAPig+cQ7Xn_DXG1NSyaktzybNPxiNBgRe=qeuqxrm9Z+GxCROQ@mail.gmail.com>
In-Reply-To
<xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com>
On Tue, Jan 9, 2018 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> When merging another branch into ours, if their tree is the same as
> the common ancestor's, we can declare that our tree represents the
> result of three-way merge.  In such a case, the recursive merge
> backend incorrectly used to create a commit out of our index, even
> when the index has changes.
>
> A recent fix attempted to prevent this by adding a comparison
> between "our" tree and the index, but forgot that this check must be
> restricted only to the outermost merge.  Inner merges performed by
> the recursive backend across merge bases are by definition made from
> scratch without having any local changes added to the index.  The
> call to index_has_changes() during an inner merge is working on the
> index that has no relation to the merge being performed, preventing
> legitimate merges from getting carried out.
>
> Fix it by limiting the check to the outermost merge.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> @@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names of all base trees' '
> +test_expect_success 'merge-recursive internal merge resolves to the sameness' '
> +       git reset --hard HEAD &&
> +
> +       # We are going to create a history leading to two criss-cross
> +       # branches A and B.  The common ancestor at the bottom, O0,
> +       # has two childs O1 and O2, both of which will be merge base
s/childs/children,/
Show 10 quoted lines
> +       # between A and B, like so:
> +       #
> +       #       O1---A
> +       #      /  \ /
> +       #    O0    .
> +       #      \  / \
> +       #       O2---B
> +       #
> +       # The recently added "check to see if the index is different from
> +       # the tree into which something else is getting merged into and
Too many "into"s: s/merged into/merged/
Show 6 quoted lines
> +       # reject" check must NOT kick in when an inner merge between O1
> +       # and O2 is made.  Both O1 and O2 happen to have the same tree
> +       # as O0 in this test to trigger the bug---whether the inner merge
> +       # is made by merging O2 into O1 or O1 into O2, their common ancestor
> +       # O0 and the branch being merged have the same tree, and the code
> +       # before fix will incorrectly try to look at the index.
Nit: Does "code before fix" belong in this comment? It sounds more
like something you'd say in the commit message.
Show 9 quoted lines
> +
> +       echo "zero" >file &&
> +       git add file &&
> +       test_tick &&
> +       git commit -m "O0" &&
> +       O0=$(git rev-parse HEAD) &&
> +
> +       test_tick &&
> +       git commit --allow-empty -m "O2" &&
s/O2/O1/
Show 18 quoted lines
> +       O1=$(git rev-parse HEAD) &&
> +
> +       git reset --hard $O0 &&
> +       test_tick &&
> +       git commit --allow-empty -m "O2" &&
> +       O2=$(git rev-parse HEAD) &&
> +
> +       test_tick &&
> +       git merge -s ours $O1 &&
> +       B=$(git rev-parse HEAD) &&
> +
> +       git reset --hard $O1 &&
> +       test_tick &&
> +       git merge -s ours $O2 &&
> +       A=$(git rev-parse HEAD) &&
> +
> +       git merge $B
> +'
Previous: Junio C HamanoNext: Elijah Newren
Message 9 of 15 in “git merge commits staged files (when two trees are identical)”
  1. Andreas KreyDec 20, 2017
  2. Elijah NewrenDec 21, 2017
  3. 1/3 t6044: recursive can silently incorporate dirty changes in a mergeElijah Newren, Dec 21, 2017
  4. 3/3 merge-recursive: Avoid incorporating uncommitted changes in a mergeElijah Newren, Dec 21, 2017
  5. Junio C HamanoDec 22, 2017
  6. Junio C HamanoJan 8, 2018
  7. merge-recursive: do not look at the index during recursive mergeJunio C Hamano, Jan 9, 2018
  8. Junio C HamanoJan 9, 2018
  9. Eric SunshineJan 9, 2018
  10. Elijah NewrenJan 9, 2018
  11. Junio C HamanoJan 9, 2018
  12. 2/3 move index_has_changes() from builtin/am.c to merge.c for reuseElijah Newren, Dec 21, 2017
  13. Elijah NewrenDec 21, 2017
  14. Junio C HamanoDec 22, 2017
  15. Elijah NewrenDec 23, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.