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

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

[Ben Peart]

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:


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,

Ben

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

[Elijah Newren]

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

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

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

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?

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

[Elijah Newren]

On Fri, Jul 20, 2018 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> 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;
>
> 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?

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

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

[Junio C Hamano]

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.

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

[Elijah Newren]

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

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

[Ben Peart]

On 7/20/2018 7:02 PM, Elijah Newren wrote:
> 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...
> 

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

[PATCH 0/2] Preserve skip_worktree bit in merges when necessary

[Elijah Newren]

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(+)

-- 
2.18.0.234.g2d1e6cefb

[PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Elijah Newren]

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(+)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 7c5ad0862..25fac490d 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_failure '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
-- 
2.18.0.234.g2d1e6cefb

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Eric Sunshine]

On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
> diff --git 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_expect_failure 'failed cherry-pick with sparse-checkout' '
> +       pristine_detach initial &&
> +       git config core.sparseCheckout true &&

Should this be test_config()?

> +       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 &&

See question above.

> +       rm .git/info/sparse-checkout

Should this cleanup be done by test_when_finished()?

> +'

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Ben Peart]

On 7/21/2018 3:21 AM, Eric Sunshine wrote:
> On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
>> diff --git 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_expect_failure 'failed cherry-pick with sparse-checkout' '
>> +       pristine_detach initial &&
>> +       git config core.sparseCheckout true &&
> 
> Should this be test_config()?
> 

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

>> +       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 &&
> 
> See question above.
> 
>> +       rm .git/info/sparse-checkout
> 
> Should this cleanup be done by test_when_finished()?
> 

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.

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Eric Sunshine]

On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:
> On 7/21/2018 3:21 AM, Eric Sunshine wrote:
> > On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
> >> +       rm .git/info/sparse-checkout
> >
> > Should this cleanup be done by test_when_finished()?
>
> 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.

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

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Ben Peart]

On 7/23/2018 2:09 PM, Eric Sunshine wrote:
> On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:
>> On 7/21/2018 3:21 AM, Eric Sunshine wrote:
>>> On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
>>>> +       rm .git/info/sparse-checkout
>>>
>>> Should this cleanup be done by test_when_finished()?
>>
>> 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.
> 
> 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 &&
>      '
> 

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

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

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Ben Peart]

On 7/21/2018 2:34 AM, Elijah Newren wrote:
> 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.

Thanks Elijah, consider it

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


> 
>   t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 7c5ad0862..25fac490d 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_failure '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
> 

Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> On 7/21/2018 2:34 AM, Elijah Newren wrote:
>> 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.
>
> Thanks Elijah, consider it
>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
>

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

[PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary

[Elijah Newren]

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(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 113c1d696..fd74bca17 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,
 	if (mfi.clean &&
 	    was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&
 	    !df_conflict_remains) {
+		int pos;
+		struct cache_entry *ce;
+
 		output(o, 3, _("Skipped %s (merged same as existing)"), path);
 		if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
 				  0, (!o->call_depth && !is_dirty), 0))
 			return -1;
+		/*
+		 * However, add_cacheinfo() will delete the old cache entry
+		 * and add a new one.  We need to copy over any skip_worktree
+		 * flag to avoid making the file appear as if it were
+		 * deleted by the user.
+		 */
+		pos = index_name_pos(&o->orig_index, path, strlen(path));
+		ce = o->orig_index.cache[pos];
+		if (ce_skip_worktree(ce)) {
+			pos = index_name_pos(&the_index, path, strlen(path));
+			ce = the_index.cache[pos];
+			ce->ce_flags |= CE_SKIP_WORKTREE;
+		}
 		return mfi.clean;
 	}
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 25fac490d..9b1456a7c 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'failed cherry-pick with sparse-checkout' '
+test_expect_success 'failed cherry-pick with sparse-checkout' '
        pristine_detach initial &&
        git config core.sparseCheckout true &&
        echo /unrelated >.git/info/sparse-checkout &&
-- 
2.18.0.234.g2d1e6cefb

Re: [PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary

[Ben Peart]

<snip>
>   merge-recursive.c               | 16 ++++++++++++++++
>   t/t3507-cherry-pick-conflict.sh |  2 +-
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 113c1d696..fd74bca17 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,
>   	if (mfi.clean &&
>   	    was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&
>   	    !df_conflict_remains) {
> +		int pos;
> +		struct cache_entry *ce;
> +
>   		output(o, 3, _("Skipped %s (merged same as existing)"), path);
>   		if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
>   				  0, (!o->call_depth && !is_dirty), 0))
>   			return -1;
> +		/*
> +		 * However, add_cacheinfo() will delete the old cache entry
> +		 * and add a new one.  We need to copy over any skip_worktree
> +		 * flag to avoid making the file appear as if it were
> +		 * deleted by the user.
> +		 */

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

> +		pos = index_name_pos(&o->orig_index, path, strlen(path));
> +		ce = o->orig_index.cache[pos];
> +		if (ce_skip_worktree(ce)) {
> +			pos = index_name_pos(&the_index, path, strlen(path));
> +			ce = the_index.cache[pos];
> +			ce->ce_flags |= CE_SKIP_WORKTREE;
> +		}
>   		return mfi.clean;
>   	}
>   
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 25fac490d..9b1456a7c 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '
>   	test_cmp expect actual
>   '
>   
> -test_expect_failure 'failed cherry-pick with sparse-checkout' '
> +test_expect_success 'failed cherry-pick with sparse-checkout' '
>          pristine_detach initial &&
>          git config core.sparseCheckout true &&
>          echo /unrelated >.git/info/sparse-checkout &&
> 

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.

diff --git a/t/t3507-cherry-pick-conflict.sh 
b/t/t3507-cherry-pick-conflict.sh
index 9b1456a7c3..bc8863ff36 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -392,17 +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' '
+test_expect_success 'cherry-pick preserves 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 "$(git ls-files -t foo)" = "S foo" &&
         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

[PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary

[Ben Peart]

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
-- 
2.17.0.gvfs.1.123.g449c066

[PATCH v2 1/2] t3507: add a testcase showing failure with sparse checkout

[Ben Peart]

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(+)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 7c5ad08626..45ddd81bfa 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_failure 'cherry-pick preserves 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
+'
+
 test_done
-- 
2.17.0.gvfs.1.123.g449c066

[PATCH v2 2/2] merge-recursive: preserve skip_worktree bit when necessary

[Ben Peart]

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(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 113c1d6962..fd74bca173 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,
 	if (mfi.clean &&
 	    was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&
 	    !df_conflict_remains) {
+		int pos;
+		struct cache_entry *ce;
+
 		output(o, 3, _("Skipped %s (merged same as existing)"), path);
 		if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
 				  0, (!o->call_depth && !is_dirty), 0))
 			return -1;
+		/*
+		 * However, add_cacheinfo() will delete the old cache entry
+		 * and add a new one.  We need to copy over any skip_worktree
+		 * flag to avoid making the file appear as if it were
+		 * deleted by the user.
+		 */
+		pos = index_name_pos(&o->orig_index, path, strlen(path));
+		ce = o->orig_index.cache[pos];
+		if (ce_skip_worktree(ce)) {
+			pos = index_name_pos(&the_index, path, strlen(path));
+			ce = the_index.cache[pos];
+			ce->ce_flags |= CE_SKIP_WORKTREE;
+		}
 		return mfi.clean;
 	}
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 45ddd81bfa..0db166152a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick preserves sparse-checkout' '
+test_expect_success 'cherry-pick preserves sparse-checkout' '
 	pristine_detach initial &&
 	test_config core.sparseCheckout true &&
 	test_when_finished "
-- 
2.17.0.gvfs.1.123.g449c066

Re: [PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary

[Junio C Hamano]

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

> 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

Very much appreciated.  Thanks.

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

Re: [PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary

[Elijah Newren]

On Fri, Jul 27, 2018 at 5:59 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> Sending this update as Elijah is on vacation.  This only updates the test
> case based on feedback from the list.

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