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

As we were attempting to migrate to 2.18 some of our internal functional tests failed. The tests that failed were testing merge and cherry-pick when there was a merge conflict. Our tests run with sparse-checkout enabled which is what exposed the bug.

What is happening is that in merge_recursive, the skip-worktree bit is 
cleared on the cache entry but then the working directory isn't updated. 
  The end result is that git reports that the merged file has actually 
been deleted.

We've identified the patch that introduced the regression as:

commit 1de70dbd1ada0069d1b6cd6345323906cc9a9ed3
Author: Elijah Newren <newren@gmail.com>
Date:   Thu Apr 19 10:58:23 2018 -0700

     merge-recursive: fix check for skipability of working tree updates

     The can-working-tree-updates-be-skipped check has had a long and 
blemished
     history.  The update can be skipped iff:
       a) The merge is clean
       b) The merge matches what was in HEAD (content, mode, pathname)
       c) The target path is usable (i.e. not involved in D/F conflict)

I've written a test that can be used to reproduce the issue:

On Fri, Jul 20, 2018 at 12:53 PM, Ben Peart <peartben@gmail.com> wrote:

Indeed, I've never used sparse checkout before. And I've got questions related to it below...

Thanks for cooking up a testcase.  In short, you've got:
  - a one-line file that was modified on both sides
  - you explicitly set the skip-worktree bit for this file (by the
core.sparseCheckout and read-tree steps),
    suggesting that you DONT want it being written to your working tree
  - you're using -Xours to avoid what would otherwise be a conflict

So, I actually think merge-recursive is behaving correctly with
respect to the working tree here, because:
  - You said you didn't want foo in your working copy with your
sparse-checkout pattern
  - You manually nuked foo from your working copy when you called 'git
read-tree --reset -u HEAD'
  - You setup this merge such that the merge result was the same as
before the merge started.
In short, the file didn't change AND you don't want it in your working
tree, so why write it out?

To me, the bug is that merge-recursive clears the skip-worktree bit for foo when that should be left intact -- at least in this case.

But that brings up another interesting question. What if a merge *does* modify a file for which you have skip-worktree set? Previously, it'd clear the bit and write the file to the working tree, but that was by no means an explicit decision; that was just a side effect of the commands it uses. Isn't that choice wrong -- shouldn't it just update the index and continue on? Or, if there are conflicts, is that a case that is considered special where you do want the skip-worktree bit cleared and have the file written out?

I'm worried that getting skip-worktree right in merge-recursive, when it had never been considered before with that codebase, might be a little messy...

Elijah Newren <newren@gmail.com> writes:

At least in my mind, the "skip worktree" aka sparse checkout has always been "best effort" in that if Git needs to materialize a working tree file in order to carry out some operation (e.g. a merge needs conflict resolution, hence we need to give a working tree file with conflict markers to the end user) Git is free to do so.

Isn't that what happens currently?

On Fri, Jul 20, 2018 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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. (Unfortunately, that means we have to work around add_cacheinfo() for these cases.)

Elijah Newren <newren@gmail.com> writes:

Correct.

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.

On Fri, Jul 20, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

On 7/20/2018 7:02 PM, Elijah Newren wrote:

I agree with the priorities around proposed behavior with this scenario.

It would be preferred that the skip worktree bit be preserved but the behavior in 2.17 of clearing it and writing the file to the working directory is much better than the current 2.18 behavior that makes the user think they had somehow deleted the file just by doing the merge.

At this point, it isn't clear to the user what they should do to recover without causing harm to the repo.

$ git status
HEAD detached at df2a63d
You are currently cherry-picking commit 7e6d412.
   (all conflicts fixed: run "git cherry-pick --continue")
   (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes not staged for commit:
   (use "git add/rm <file>..." to update what will be committed)
   (use "git checkout -- <file>..." to discard changes in working directory)

         deleted:    foo

merge-recursive used to update files in the working tree unnecessarily. This was reported by Linus in the 2.18.0 cycle and fixed in commit 1de70dbd1 ("merge-recursive: fix check for skipability of working tree updates", 2018-04-19). Unfortunately, this bug masked another one: that merge-recursive cleared the skip_worktree bit for any files marked as unmerged by unpack_trees(), even if a file-level merge was clean.

This series fixes the clearing of the skip_worktree bit for files that merge cleanly and match HEAD. A future possible improvement exists to also avoid clearing the skip_worktree bit for files that merge cleanly but do not match HEAD (for such cases we'd still want to write those files to the index, but stop updating them in the working tree).

This series applies cleanly to either maint or master (or next or pu).

Two important notes:
  - Need a sign-off from Ben for the first patch
  - I'm out on vacation next week, so I won't be able to respond to
    feedback or handle any necessary re-rolls until I return.

Ben Peart (1):
  t3507: add a testcase showing failure with sparse checkout

Elijah Newren (1):
  merge-recursive: preserve skip_worktree bit when necessary

 merge-recursive.c               | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++
 2 files changed, 29 insertions(+)

From: Ben Peart <peartben@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Testcase provided by Ben, so committing with him as the author.  Just need
a sign off from him.

 t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:

Should this be test_config()?

See question above.

Should this cleanup be done by test_when_finished()?

On 7/21/2018 3:21 AM, Eric Sunshine wrote:

I think using test_config() here is fine but...

I think trying to use test_when_finished() for this really degrades the readability of the test. See below:

test_expect_success 'failed cherry-pick with sparse-checkout' '
	pristine_detach initial &&
	test_config core.sparsecheckout true &&
	echo /unrelated >.git/info/sparse-checkout &&
	git read-tree --reset -u HEAD &&
	test_when_finished "echo \"/*\" >.git/info/sparse-checkout && git 
read-tree --reset -u HEAD && rm .git/info/sparse-checkout" &&
	test_must_fail git cherry-pick -Xours picked>actual &&
	test_i18ngrep ! "Changes not staged for commit:" actual
'

Given it takes multiple commands, I'd prefer to keep the setup and cleanup of the sparse checkout settings symmetrical.

On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:

Some observations:

The test_when_finished() ought to be called before the initial git-read-tree, otherwise you risk leaving a .git/info/sparse-checkout sitting around if git-read-tree fails.

The tear-down code could be moved to a function, in which case, test_when_finished() would simply call that function.

Multi-line quoted strings are valid, so you don't need to string out all the tear-down steps on a single line like that, and instead spread them across multiple lines to improve readability.

test_when_finished() doesn't expect just a single quoted string as argument. In fact, it can take many (unquoted) arguments, which also allows you to spread the tear-down steps over multiple lines to improve readability.

Multiple test_when_finished() invocations are allowed, so you could spread out the tear-down commands that way (though they'd have to be in reverse order, which would be bad for readability in this case, thus not recommended).

Correctness ought to trump readability, not the other way around.

So, one possibility, which seems pretty readable to me:

    test_expect_failure 'failed cherry-pick with sparse-checkout' '
       pristine_detach initial &&
       test_config core.sparseCheckout true &&
       test_when_finished "
           echo \"/*\" >.git/info/sparse-checkout
           git read-tree --reset -u HEAD
           rm .git/info/sparse-checkout" &&
       echo /unrelated >.git/info/sparse-checkout &&
       git read-tree --reset -u HEAD &&
       test_must_fail git cherry-pick -Xours picked>actual &&
       test_i18ngrep ! "Changes not staged for commit:" actual &&
    '

Notice that I dropped the internal &&-chain in test_when_finish() to ensure that the final 'rm' is invoked even if the cleanup git-read-tree fails (though all bets are probably off, anyhow, if it does fail).

On 7/23/2018 2:09 PM, Eric Sunshine wrote:

Minus the trailing && on the last line, that works for me. Thank you - readability and correctness.

On 7/21/2018 2:34 AM, Elijah Newren wrote:

Thanks Elijah, consider it

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>

Ben Peart <peartben@gmail.com> writes:

Thanks; I'll also tweak the in-body From: line while applying to match the Sign-off.

merge-recursive takes any files marked as unmerged by unpack_trees, tries to figure out whether they can be resolved (e.g. using renames or a file-level merge), and then if they can be it will delete the old cache entries and writes new ones. This means that any ce_flags for those cache entries are essentially cleared when merging.

Unfortunately, if a file was marked as skip_worktree and it needs a file-level merge but the merge results in the same version of the file that was found in HEAD, we skip updating the worktree (because the file was unchanged) but clear the skip_worktree bit (because of the delete-cache-entry-and-write-new-one). This makes git treat the file as having a local change in the working copy, namely a delete, when it should appear as unchanged despite not being present. Avoid this problem by copying the skip_worktree flag in this case.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
No need to check whether pos >= 0 in this patch because the fact that
we got to this point in the code meant the entry was definitely in
both the new and old indexes (and with the same oid and mode).

We could optimize this a bit; the call to was_tracked_and_matches() already does the lookup in o->orig_index. So we could cache that and re-use it. Likewise, if we instead set ce_flags just after calling make_cache_entry() within add_cacheinfo(), we could avoid looking up the cache entry in the_index as well. Setting ce_flags there would just require plumbing an extra flag option through add_cacheinfo() and modifying all other callsites to pass 0 for that flag. But doing all this felt a little messy, and I really wanted to keep the logic for this case all in one little place. Especially for a fixup that might be wanted for maint.

There is also another callsite in update_file_flags() that could be updated to preserve the skip_worktree flag, which would be technically better. But I really don't want to tackle that right now, I just want a small simple fix for Ben's issue.

Besides, as Junio said:

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

 merge-recursive.c               | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

<snip>

nit - I find it a little odd to start a comment with "However" as if you are continuing a conversation.

Thanks Elijah, I can verify this fixes the problem with the preferred solution (ie it preserves the skip-worktree bit). As such, this is:

Reviewed-by: Ben Peart <benpeart@microsoft.com>

That said, I would propose the test be updated to include a specific test for the skip-worktree bit so that if a future patch reverts to the old behavior of clearing the skip-worktree bit and writing out the file to the working directory, we do it explicitly instead of by accident.

Sending this update as Elijah is on vacation. This only updates the test case based on feedback from the list.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/0ab3816d61
Checkout: git fetch https://github.com/benpeart/git merge-recursive-v2 && git checkout 0ab3816d61

### Patches

Ben Peart (1):
  t3507: add a testcase showing failure with sparse checkout

Elijah Newren (1):
  merge-recursive: preserve skip_worktree bit when necessary

 merge-recursive.c               | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++
 2 files changed, 29 insertions(+)

base-commit: ffc6fa0e396238de3a30623912980263b4f283ab

From: Ben Peart <peartben@gmail.com>

Recent changes in merge_content() induced a bug when merging files that are not present in the local working directory due to sparse-checkout. Add a test case to demonstrate the bug so that we can ensure the fix resolves it and to prevent future regressions.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

From: Elijah Newren <newren@gmail.com>

merge-recursive takes any files marked as unmerged by unpack_trees, tries to figure out whether they can be resolved (e.g. using renames or a file-level merge), and then if they can be it will delete the old cache entries and writes new ones. This means that any ce_flags for those cache entries are essentially cleared when merging.

Unfortunately, if a file was marked as skip_worktree and it needs a file-level merge but the merge results in the same version of the file that was found in HEAD, we skip updating the worktree (because the file was unchanged) but clear the skip_worktree bit (because of the delete-cache-entry-and-write-new-one). This makes git treat the file as having a local change in the working copy, namely a delete, when it should appear as unchanged despite not being present. Avoid this problem by copying the skip_worktree flag in this case.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c               | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

Ben Peart <Ben.Peart@microsoft.com> writes:

Very much appreciated.  Thanks.

On Fri, Jul 27, 2018 at 5:59 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:

Thanks!  One less thing for me to catch up on.  :-)