Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree

[Elijah Newren <newren@gmail.com>]

On Fri, Jul 20, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> Ah, okay, that's helpful.  So, if there are conflicts, it should be
>> free to clear the skip_worktree flag.  Since merge-recursive calls
>> add_cacheinfo() for all entries it needs to update, which deletes the
>> old cache entry and just makes new ones, we get that for free.
>
> Correct.
>
>> And conversely, if a file-level merge succeeds without conflicts then
>> it clearly doesn't "need to materialize a working tree file", so it
>> should NOT clear the skip_worktree flag for that path.
>
> That is not at all implied by what I wrote, though.
>
> If it can be done without too much effort, then it certainly is
> nicer to keep the sparseness when we do not have to materialize the
> working tree file.  But at least in my mind, if it needs too many
> special cases, hacks, and conditionals, then it is not worth the
> complexity---if it is easier to write a correct code by allowing Git
> to populate working tree files, it is perfectly fine to do so.
>
> In a sense, the sparse checkout "feature" itself is a hack by
> itself, and that is why I think this part should be "best effort" as
> well.

That's good to know, but I don't think we can back out easily:
  - Clearing the skip_worktree bit: no big deal, as you mention above
  - Avoiding working tree updates when merge doesn't change them: very
desirable[1]
  - Doing both: whoops

[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/


I don't want to regress the bug Linus reported, so to fix Ben's issue,
when we detect that a path's contents/mode won't be modified by the
merge, we can either:
  - Update the working tree file if the original cache entry had the
skip_worktree flag set
  - Mark the new cache entry as skip_worktree if the original cache
entry had the skip_worktree flag set

Both should be about the same amount of work; the first seems weird
and confusing for future readers of the code.  The second makes sense,
but probably should be accompanied with a note in the code about how
there are other codepaths that could consider skip_worktree too.

I'll see if I can put something together, but I have family flying in
tomorrow, and then am out on vacation Mon-Sat next week, so...