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

From: Elijah Newren <newren@gmail.com>
Date: Jul 20, 2018, 20:48 UTC
Message-ID: <CABPp-BF+Vx8YT2KAJQ+szbkYExv-_o5E-ZkywgvzsHWR0QvVEg@mail.gmail.com>
In-Reply-To: <5a8d1098-b4c5-64e1-da98-dac13521e7ba@gmail.com>

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

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

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

Show 49 quoted lines

> 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:
>
>
> diff --git a/t/t3507-cherry-pick-conflict.sh
> b/t/t3507-cherry-pick-conflict.sh
> index 7c5ad08626..de0bdc8634 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the
> sign-off at the right place' '
>
>         test_cmp expect actual
>  '
>
> +test_expect_success 'failed cherry-pick with sparse-checkout' '
> +       pristine_detach initial &&
> +       git config core.sparseCheckout true &&
> +       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 &&
> +       echo "/*" >.git/info/sparse-checkout &&
> +       git read-tree --reset -u HEAD &&
> +       git config core.sparseCheckout false &&
> +       rm .git/info/sparse-checkout
> +'
> +
>  test_done

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