mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <>
To: Ben Peart <>
Cc: Git Mailing List <>,
	Kevin Willford <>,
	Ben Peart <>
Subject: Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree
Date: Fri, 20 Jul 2018 13:48:37 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Jul 20, 2018 at 12:53 PM, Ben Peart <> 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...

> 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 <>
> 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/
> b/t/
> index 7c5ad08626..de0bdc8634 100755
> --- a/t/
> +++ b/t/
> @@ -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...

  reply	other threads:[~2018-07-20 20:48 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-20 19:53 [BUG] merge-recursive overly aggressive when skipping updating the working tree Ben Peart
2018-07-20 20:48 ` Elijah Newren [this message]
2018-07-20 21:13   ` Junio C Hamano
2018-07-20 21:42     ` Elijah Newren
2018-07-20 22:05       ` Junio C Hamano
2018-07-20 23:02         ` Elijah Newren
2018-07-23 12:49           ` Ben Peart
2018-07-21  6:34 ` [PATCH 0/2] Preserve skip_worktree bit in merges when necessary Elijah Newren
2018-07-21  6:34   ` [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout Elijah Newren
2018-07-21  7:21     ` Eric Sunshine
2018-07-23 13:12       ` Ben Peart
2018-07-23 18:09         ` Eric Sunshine
2018-07-23 18:22           ` Ben Peart
2018-07-21 13:02     ` Ben Peart
2018-07-23 18:12       ` Junio C Hamano
2018-07-21  6:34   ` [PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary Elijah Newren
2018-07-23 14:14     ` Ben Peart
2018-07-27 12:59 ` [PATCH v2 0/2] Preserve skip_worktree bit in merges " Ben Peart
2018-07-27 12:59   ` [PATCH v2 1/2] t3507: add a testcase showing failure with sparse checkout Ben Peart
2018-07-27 12:59   ` [PATCH v2 2/2] merge-recursive: preserve skip_worktree bit when necessary Ben Peart
2018-07-27 18:14   ` [PATCH v2 0/2] Preserve skip_worktree bit in merges " Junio C Hamano
2018-07-31 16:11   ` Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).