Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths

[Elijah Newren <newren@gmail.com>]

On Sun, Apr 15, 2018 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,
>>       init_tree_desc_from_tree(t+2, merge);
>>
>>       rc = unpack_trees(3, t, &o->unpack_opts);
>> +     cache_tree_free(&active_cache_tree);
>> +
>> +     o->orig_index = the_index;
>> +     the_index = tmp_index;
>> +
>>       /*
>> -      * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
>> -      * so set to the new index which will usually have modification
>> -      * timestamp info copied over.
>> +      * src_index is used in verify_uptodate, but was NULLified in
>> +      * unpack_trees, so we need to set it back to the original index.
>>        */
>
> Was NULLified?  I thought that the point of src/dst distinction
> Linus introduced long time ago at 34110cd4 ("Make 'unpack_trees()'
> have a separate source and destination index", 2008-03-06) was that
> we can then keep the source side of the traversal unmodified.

That comment is messed up; maybe I edited and re-edited the comment
multiple times and then didn't notice the big problems when
re-reading?

Anyway, I should move the comment a few lines up, and make the code
instead read:

    /*
     * Update the_index to match the new results, AFTER saving a copy
     * in o->orig_index.  Update src_index to point to the saved copy.
     * (verify_uptodate() checks src_index, and the original index is
     * the one that had the necessary modification timestamps.)
     */
    o->orig_index = the_index;
    the_index = tmp_index;
    o->unpack_opts.src_index = &o->orig_index;

>>  static int would_lose_untracked(const char *path)
>>  {
>> -     return !was_tracked(path) && file_exists(path);
>> +     /*
>> +      * This may look like it can be simplified to:
>> +      *   return !was_tracked(o, path) && file_exists(path)
>> +      * but it can't.  This function needs to know whether path was
>> +      * in the working tree due to EITHER having been tracked in the
>> +      * index before the merge OR having been put into the working copy
>> +      * and index by unpack_trees().  Due to that either-or requirement,
>> +      * we check the current index instead of the original one.
>> +      */
>
> If this path was created by merge-recursive, not by unpack_trees(),
> what does this function want to say?  Say, we are looking at path P,
> the other branch we are merging moved some other path Q to P (while
> our side modified contents at path Q).  Then path P we are looking
> at has contents of Q at the merge base at stage #1, the contents of
> Q from our HEAD at stage #2 and the contents of P from the other
> branch at stage #3.  The code below says "path P is OK, we won't
> lose it" in such a case, but it is unclear if the above comment
> wants to also cover that case.

Oh, boy, here be dragons...

The comment as-is actually does cover your example case with Q and P:
unpack_trees(), which is unaware of renames, will see that P only
exists on one side of history and thus load it into the index at stage
0 rather than stage 3.

But your general comment about whether something else in
merge-recursive could create a path in the current index after
unpack_trees() is interesting...it touches on a pitfall that has bit
me multiple times.  There is a required ordering in merge-recursive.c
that for any given path, the working directory must be updated before
the index is -- otherwise, would_lose_untracked() will return faulty
information.  update_file_flags() has this ordering builtin,
update_stages() has a big obnoxious comment at the beginning about how
it should not be called until after update_file() is,
apply_directory_rename_modifications() has a big comment about this
~80% of the way through the function (look for
"would_lose_untracked"), and conflict_rename_rename_2to1() has a big
obnoxious comment near the end painstakingly pointing out that some
code that feels like it would make more sense being combined with a
previous function cannot be due to the
update-working-directory-before-index requirement.

I should probably add to this comment something about this annoying
(and error-prone) ordering restriction, since this function is the
source of those particular pains.  Your suggested ideal-world rewrite
(run unpack_trees() with unpack_opts.index_only=1, do merge in memory,
then update working tree), would make this whole problem go away.