From: Ben Peart <peartben@gmail.com>
Date: Mon, 23 Jul 2018 14:14:40 GMT
Subject: Re: [PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary
Message-ID: <75aa297e-4857-d92a-7041-618ff3b0b77a@gmail.com>
In-Reply-To: <20180721063428.20518-3-newren@gmail.com>



<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