On Wed, Aug 3, 2011 at 6:20 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Unfortunately I seem to have found a regression that manifests in the real
> life.
>
> When this series is merged to 'next', it mismerges a trivial renamed path.
I know the problem, and can think of a couple potential solutions but
I'm not sure which one of those (if any) should be used. I may need
advice from an unpack_trees expert.
This breakage was introduced in d487486 (merge-recursive: When we
detect we can skip an update, actually skip it 2011-06-08), and the
reason for the breakage is that was_tracked(path) is lying to us. Let
me explain...
If you run with GIT_MERGE_VERBOSITY=3, you'll note that you get the message
"Skipped two (merged same as existing)"
which is NOT the path it should be taking. The reason for this is
that the call to was_tracked('two') just before that is returning true
for us when the correct answer is false. Explaining why we get the
wrong answer requires understanding a couple things.
Using your simple reproduction recipe as an example to explain this,
the sha1sums and paths for your merge case are:
Base commit: d00491f... one
HEAD commit: 0cfbf08... one
side commit: d00491f... two
In other words, 'one' is modified in HEAD, and unchanged other than
being renamed (to 'two') on side.
Now, was_tracked() simply checks whether there is a cache entry in the
current index with either stage 0 or stage 2. Clearly, I was
expecting 'two' to only appear with stage 3, but that's not what
happened.
When unpacking these trees, we got three cache entries: one for path
'one' at stage 1, one for path 'one' at stage 2, and one for path
'two' at stage 0. Yes, stage 0 and not stage 3. The reason for this
is that this is case 2ALT from
Documentation/technical/trivial-merge.txt:
case ancest head remote result
----------------------------------------
...
2ALT (empty)+ *empty* remote remote
This means the threeway_merge code considers it a clean merge and
calls merged_entry on it, which clears the CE_STAGEMASK bits from the
cache entry flags.
Since the cache entry for path 'two' has the CE_STAGEMASK bits
cleared, when we call was_tracked on that path, we get back the answer
true. 'two' was not tracked in HEAD, though, so this is the wrong
answer and the source of the bug.
Now, the question is...how do we fix the was_tracked function? It
would be nice to make use of the original index we had before
unpacking, but that is overwritten at the end of unpack_trees. Should
we save it somewhere? Or should we save off all the tracked filenames
before unpacking into a linked list and then have was_tracked() use
that instead of looking it up in the current cache? (I'm slightly
worried that may be slow given the number of lookups that exist).
Thoughts?
Elijah
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html