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

From: Elijah Newren <newren@gmail.com>
Date: Mar 2, 2011, 22:22 UTC
Message-ID: <AANLkTin1EDUMRY7T+Jx64eG6R4mOZOeOukJWQHpFX+j5@mail.gmail.com>
In-Reply-To: <7vwrkhb7ig.fsf@alter.siamese.dyndns.org>

On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> 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.

Show 6 quoted lines

> 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?