Re: [PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it

[Elijah Newren <newren@gmail.com>]

On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> @@ -1274,9 +1275,13 @@ static int merge_content(struct merge_options *o,
>>       }
>>
>>       if (mfi.clean && !df_conflict_remains &&
>> -         sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode)
>> +         sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&
>> +         lstat(path, &st) == 0) {
>>               output(o, 3, "Skipped %s (merged same as existing)", path);
>> -     else
>> +             add_cacheinfo(mfi.mode, mfi.sha, path,
>> +                           0 /*stage*/, 1 /*refresh*/, 0 /*options*/);
>> +             return mfi.clean;
>> +     } else
>
> Hmmmm.  During a merge, we allow files missing from the working tree as if
> it is not modified from the index.  Changing the behaviour based on the
> existence of the path on the filesystem does not feel quite right.

Really?  Ouch.  Then things have been broken ever since
make_room_for_directories_of_df_conflicts() was introduced, as that
function deletes files intentionally and expects them to be reinstated
later when possible.  If a user has a file deleted that happens to be
involved in a D/F conflict before a merge, and the file is unchanged,
the merge will reinstate it.

> Even if we decide that regressing in that use case were acceptable, what
> guarantees that the path that happens to be in the work tree has the same
> contents as what the merge result should be?  IOW, shouldn't we be looking
> at the original index, making sure that our side (stage #2) at the path
> had a file there that matches the merge result mfi.{sha,mode}, instead of
> looking at the working tree?

We DID look at the original index, make sure that our side (stage #2)
at the path had a file there that matches the merge result
mfi.{sha,mode}.

But, that's not enough to know that we can skip the update of the
file.  The merged results being different are only one reason to
update the file; the other is that we may have deleted the file
ourselves in make_room_for_directories_of_df_conflicts() and need to
replace it.  This is the location in the code where such deleted files
are reinstated.


Suggestions?