Re: [BUG] `rerere remaining` skips consecutive conflicted paths

[Junio C Hamano <gitster@pobox.com>]

Mikko Rantalainen <mikko.rantalainen@peda.net> writes:

> The issue is probably  caused by `check_one_conflict()` in `rerere.c.
> There is currently a loop of the form:
>
> ```
> *type = PUNTED;
> while (i < istate->cache_nr && ce_stage(istate->cache[i]) == 1)
>          i++;
> ```
>
> According to ChatGPT, this is probably intended to skip multiple stage-1
> entries belonging to the same conflicted pathname, but it also skips a
> stage-1 entry belonging to the next pathname.
>
> The loop may need an additional same-path check, maybe
> something like:
>
> ```
> while (i < istate->cache_nr &&
>         ce_stage(istate->cache[i]) == 1 &&
>         ce_same_name(e, istate->cache[i]))
>          i++;
> ```
>
> I have not checked whether `ce_same_name()` is necessarily the
> preferred helper here, so this is only a possible fix rather than
> a proposed patch.

Spot on, I would say, even though I find that it is a bit iffy for
the merge machinery to leave a "delete-delete" conflict in the first
place.

The idea of that function is to return for the current path if we
(1) don't need to do anything as it is cleanly resolved (RESOLVED),
(2) know it is conflicting but we cannot handle (PUNTED), or (3)
know it is conflicting and we are willing to handle (THREE_STAGED).

For (1), we only need to see that the current entry is resolved
(because in istate->cache[], resolved entry for a single path
appears only once) and return, telling the caller that we consumed
only one entry.  For THREE_STAGED, we would want to see a stage 2
(i.e., ours) entry followed by a stage 3 (i.e., theirs) entry, and
the way the code does so is to skip over stage 1 entries for the
same path, and we must see stage 2 and then stage 3 entries after
that.  Again in istate->cache[], by definition more than one stage 2
entries (i.e., "ours") cannot exist for a single path, so we check
if the first entry after skipping over the stage 1 entries (i.e.,
"common") is a stage 2 entry and immediately after that is a stage 3
entry, and the stage 3 entry has the same name as the first entry
we started looking at upon entry to the function.  And to conclude
one iteration, we skip the entries of the same name at the end.

And as you pointed out, the same "must be the same name" check must
be done also while we are skipping over stage 1 entries.  If you
have a sequence of stage 1 entries for different paths, all of them
would probably be skipped over at once.

Note that the low-level merge machinery and rerere machinery are
both prepared to see multiple stage #1 and stage #3 entries for a
same path, even though multiple stage #0 and stage #2 entries is a
sign of index corruption.  The "resolve" merge strategy will use
multiple stage #1 entries when dealing with a criss-cross merges,
where multiple merge-bases exist.  Being prepared for multiple stage
#3 entries is purely for philosophical consistency---an Octopus merge
ought to be representing more than one "their" branches as stage #3
entries, even though the current implementation of octopus merge of
N branches happens to do N pair-wise merges and do not require
multiple stage #3 entries.

 rerere.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git c/rerere.c w/rerere.c
index 1c3745d9e3..296f254c1e 100644
--- c/rerere.c
+++ w/rerere.c
@@ -499,7 +499,11 @@ static int check_one_conflict(struct index_state *istate, int i, int *type)
 	}
 
 	*type = PUNTED;
-	while (i < istate->cache_nr && ce_stage(istate->cache[i]) == 1)
+
+	/* First ignore stage #1 entries */
+	while (i < istate->cache_nr &&
+	       ce_same_name(e, istate->cache[i]) &&
+	       ce_stage(istate->cache[i]) == 1)
 		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */