From: Elijah Newren Date: Wed, 02 Mar 2011 22:22:49 GMT Subject: Re: [PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it Message-ID: In-Reply-To: <7vwrkhb7ig.fsf@alter.siamese.dyndns.org> On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano wrote: > Elijah Newren 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?