Re: [PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary
- From
Ben Peart <peartben@gmail.com>
- Date
- Jul 23, 2018, 14:14 UTC
- Message-ID
- <75aa297e-4857-d92a-7041-618ff3b0b77a@gmail.com>
- In-Reply-To
- <20180721063428.20518-3-newren@gmail.com>
<snip>
Show 25 quoted lines
> 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.
Show 24 quoted lines
> + 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