Re: Optimizing writes to unchanged files during merges?

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Apr 12, 2018, 23:55 UTC
Message-ID: <CA+55aFw5mpEcEpPTOWych-kjNLc8pEn8FdjJHe2u7HUBBLy-Fw@mail.gmail.com>
In-Reply-To: <CA+55aFwM2CaafNGq8_=GkYAw9inpm-4xcyHUmKprLv4Gb3-aVg@mail.gmail.com>

[ Talking to myself ]

On Thu, Apr 12, 2018 at 4:41 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 9 quoted lines

>
> Oddly, that *already* has the check:
>
>         if (mfi.clean && !df_conflict_remains &&
>             oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {
>                 int path_renamed_outside_HEAD;
>                 output(o, 3, _("Skipped %s (merged same as existing)"), path);
>
> but that doesn't seem to actually trigger for some reason.

Actually, that check triggers just fine.

> But the code really seems to have the _intention_ of skipping the case
> where the result ended up the same as the source.
>
> Maybe I'm missing something.

The later check that does

                /*
                 * The content merge resulted in the same file contents we
                 * already had.  We can return early if those file contents
                 * are recorded at the correct path (which may not be true
                 * if the merge involves a rename).
                 */
                path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
                if (!path_renamed_outside_HEAD) {

will see that 'path2' is NULL, and not trigger this early out case, and then this all falls back to the normal cases after all.

So I think that 'path_renamed_outside_HEAD' logic is somehow broken.

Did it perhaps mean to say

                path_renamed_outside_HEAD = path2 && !strcmp(path, path2);

instead?

See commit 5b448b853 ("merge-recursive: When we detect we can skip an update, actually skip it") which really implies we want to actually skip it (but then we don't anyway).

Also see commit b2c8c0a76 ("merge-recursive: When we detect we can skip an update, actually skip it") which was an earlier version, and which *actually* skipped it, but it was reverted because of that rename issue.

Adding Elijah Newren to the cc, because he was working on this back then, an dis still around, and still working on merging ;)

               Linus