git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Derrick Stolee <dstolee@microsoft.com>,
	Elijah Newren <newren@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: [PATCH v3 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
Date: Fri, 27 Mar 2020 00:49:00 +0000
Message-ID: <844306c3e86ef67591cc086decb2b760e7d710a3.1585270142.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.726.v3.git.git.1585270142.gitgitgadget@gmail.com>

From: Elijah Newren <newren@gmail.com>

Setting and clearing of the SKIP_WORKTREE bit is not only done when
users run 'sparse-checkout'; other commands such as 'checkout' also run
through unpack_trees() which has logic for handling this special bit.
As such, we need to consider how they handle special cases.  A couple
comparison points should help explain the rationale for changing how
unpack_trees() handles these bits:

    Ignoring sparse checkouts for a moment, if you are switching
    branches and have dirty changes, it is only considered an error that
    will prevent the branch switching from being successful if the dirty
    file happens to be one of the paths with different contents.

    SKIP_WORKTREE has always been considered advisory; for example, if
    rebase or merge need or even want to materialize a path as part of
    their work, they have always been allowed to do so regardless of the
    SKIP_WORKTREE setting.  This has been used for unmerged paths, but
    it was often used for paths it wasn't needed just because it made
    the code simpler.  It was a best-effort consideration, and when it
    materialized paths contrary to the SKIP_WORKTREE setting, it was
    never required to even print a warning message.

In the past if you trying to run e.g. 'git checkout' and:
  1) you had a path that was materialized and had some dirty changes
  2) the path was listed in $GITDIR/info/sparse-checkout
  3) this path did not different between the current and target branches
then despite the comparison points above, the inability to set
SKIP_WORKTREE was treated as a *hard* error that would abort the
checkout operation.  This is completely inconsistent with how
SKIP_WORKTREE is handled elsewhere, and rather annoying for users as
leaving the paths materialized in the working copy (with a simple
warning) should present no problem at all.

Downgrade any errors from inability to toggle the SKIP_WORKTREE bit to a
warning and allow the operations to continue.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh | 11 +++++-----
 t/t2018-checkout-branch.sh           | 22 ++++++++++++++++++++
 unpack-trees.c                       | 31 ++++++++++++++--------------
 3 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index eb44bafb593..63223e13bd1 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -233,18 +233,19 @@ test_expect_success 'read-tree --reset removes outside worktree' '
 	test_must_be_empty result
 '
 
-test_expect_success 'print errors when failed to update worktree' '
+test_expect_success 'print warnings when some worktree updates disabled' '
 	echo sub >.git/info/sparse-checkout &&
 	git checkout -f init &&
 	mkdir sub &&
 	touch sub/added sub/addedtoo &&
-	test_must_fail git checkout top 2>actual &&
+	# Use -q to suppress "Previous HEAD position" and "Head is now at" msgs
+	git checkout -q top 2>actual &&
 	cat >expected <<\EOF &&
-error: The following untracked working tree files would be overwritten by checkout:
+warning: The following paths were already present and thus not updated despite sparse patterns:
 	sub/added
 	sub/addedtoo
-Please move or remove them before you switch branches.
-Aborting
+
+After fixing the above paths, you may want to run `git sparse-checkout reapply`.
 EOF
 	test_i18ncmp expected actual
 '
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index bbca7ef8da6..21583154d8e 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -238,4 +238,26 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE
 	test_path_is_file dest/a.t
 '
 
+test_expect_success 'checkout -b to a new branch preserves mergeable changes despite sparse-checkout' '
+	test_when_finished "
+		git reset --hard &&
+		git checkout branch1-scratch &&
+		test_might_fail git branch -D branch3 &&
+		git config core.sparseCheckout false &&
+		rm .git/info/sparse-checkout" &&
+
+	test_commit file2 &&
+
+	echo stuff >>file1 &&
+	echo file2 >.git/info/sparse-checkout &&
+	git config core.sparseCheckout true &&
+
+	CURHEAD=$(git rev-parse HEAD) &&
+	do_checkout branch3 $CURHEAD &&
+
+	echo file1 >expect &&
+	git diff --name-only >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index dec044339df..b43f3e775ad 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1701,23 +1701,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			 * correct CE_NEW_SKIP_WORKTREE
 			 */
 			if (ce->ce_flags & CE_ADDED &&
-			    verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) {
-				if (!o->show_all_errors)
-					goto return_failed;
-				ret = -1;
-			}
+			    verify_absent(ce, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, o))
+				ret = 1;
+
+			if (apply_sparse_checkout(&o->result, ce, o))
+				ret = 1;
 
-			if (apply_sparse_checkout(&o->result, ce, o)) {
-				if (!o->show_all_errors)
-					goto return_failed;
-				ret = -1;
-			}
 			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;
-
 		}
-		if (ret < 0)
-			goto return_failed;
 		/*
 		 * Sparse checkout is meant to narrow down checkout area
 		 * but it does not make sense to narrow down to empty working
@@ -1728,6 +1720,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
 			goto done;
 		}
+		if (ret == 1) {
+			/*
+			 * Inability to sparsify or de-sparsify individual
+			 * paths is not an error, but just a warning.
+			 */
+			if (o->show_all_errors)
+				display_warning_msgs(o);
+			ret = 0;
+		}
 	}
 
 	ret = check_updates(o, &o->result) ? (-2) : 0;
@@ -1759,10 +1760,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	return ret;
 
 return_failed:
-	if (o->show_all_errors) {
+	if (o->show_all_errors)
 		display_error_msgs(o);
-		display_warning_msgs(o);
-	}
 	mark_all_ce_unused(o->src_index);
 	ret = unpack_failed(o, NULL);
 	if (o->exiting_early)
-- 
gitgitgadget


  parent reply index

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-14  7:11 [PATCH 0/7] Sparse checkout improvements -- improved sparsity updating Elijah Newren via GitGitGadget
2020-03-14  7:11 ` [PATCH 1/7] unpack-trees: fix minor typo in comment Elijah Newren via GitGitGadget
2020-03-14  7:11 ` [PATCH 2/7] t1091: make some tests a little more defensive against failures Elijah Newren via GitGitGadget
2020-03-14  7:11 ` [PATCH 3/7] unpack-trees: allow check_updates() to work on a different index Elijah Newren via GitGitGadget
2020-03-14  7:11 ` [PATCH 4/7] unpack-trees: do not mark a dirty path with SKIP_WORKTREE Elijah Newren via GitGitGadget
2020-03-15 14:39   ` Derrick Stolee
2020-03-14  7:11 ` [PATCH 5/7] unpack-trees: add a new update_sparsity() function Elijah Newren via GitGitGadget
2020-03-15 18:17   ` Derrick Stolee
2020-03-16 20:24     ` Elijah Newren
2020-03-14  7:11 ` [PATCH 6/7] sparse-checkout: use " Elijah Newren via GitGitGadget
2020-03-15 16:19   ` Derrick Stolee
2020-03-16 17:02     ` Elijah Newren
2020-03-14  7:11 ` [PATCH 7/7] sparse-checkout: provide a new update subcommand Elijah Newren via GitGitGadget
2020-03-15 16:24   ` Derrick Stolee
2020-03-16 17:05     ` Elijah Newren
2020-03-16 17:18       ` Derrick Stolee
2020-03-16 19:23         ` Elijah Newren
2020-03-15 12:26 ` [PATCH 0/7] Sparse checkout improvements -- improved sparsity updating Derrick Stolee
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 01/18] unpack-trees: fix minor typo in comment Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 02/18] unpack-trees: remove unused error type Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
2020-03-23 15:57     ` Derrick Stolee
2020-03-21 17:59   ` [PATCH v2 05/18] t1091: make some tests a little more defensive against failures Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 06/18] unpack-trees: allow check_updates() to work on a different index Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 07/18] unpack-trees: do not mark a dirty path with SKIP_WORKTREE Elijah Newren via GitGitGadget
2020-03-21 17:59   ` [PATCH v2 08/18] unpack-trees: pull sparse-checkout pattern reading into a new function Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function Elijah Newren via GitGitGadget
2020-03-23 18:02     ` Derrick Stolee
2020-03-23 18:10       ` Elijah Newren
2020-03-23 18:21         ` Derrick Stolee
2020-03-23 20:24           ` Junio C Hamano
2020-03-21 18:00   ` [PATCH v2 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
2020-03-23 18:07     ` Derrick Stolee
2020-03-23 18:14       ` Elijah Newren
2020-03-23 18:22         ` Derrick Stolee
2020-03-21 18:00   ` [PATCH v2 11/18] sparse-checkout: use improved unpack_trees porcelain messages Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 12/18] unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 13/18] unpack-trees: rename ERROR_* fields meant for warnings to WARNING_* Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 14/18] unpack-trees: split display_error_msgs() into two Elijah Newren via GitGitGadget
2020-03-23 18:32     ` Derrick Stolee
2020-03-21 18:00   ` [PATCH v2 15/18] unpack-trees: make sparse path messages sound like warnings Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 16/18] unpack-trees: provide warnings on sparse updates for unmerged paths too Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning Elijah Newren via GitGitGadget
2020-03-21 18:00   ` [PATCH v2 18/18] sparse-checkout: provide a new reapply subcommand Elijah Newren via GitGitGadget
2020-03-23 18:40     ` Derrick Stolee
2020-03-23 18:41   ` [PATCH v2 00/18] Sparse checkout improvements -- improved sparsity updating Derrick Stolee
2020-03-23 20:26     ` Junio C Hamano
2020-03-27  0:48   ` [PATCH v3 " Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 01/18] unpack-trees: fix minor typo in comment Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 02/18] unpack-trees: remove unused error type Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 05/18] t1091: make some tests a little more defensive against failures Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 06/18] unpack-trees: allow check_updates() to work on a different index Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 07/18] unpack-trees: do not mark a dirty path with SKIP_WORKTREE Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 08/18] unpack-trees: pull sparse-checkout pattern reading into a new function Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 09/18] unpack-trees: add a new update_sparsity() function Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 11/18] sparse-checkout: use improved unpack_trees porcelain messages Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 12/18] unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 13/18] unpack-trees: rename ERROR_* fields meant for warnings to WARNING_* Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 14/18] unpack-trees: split display_error_msgs() into two Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 15/18] unpack-trees: make sparse path messages sound like warnings Elijah Newren via GitGitGadget
2020-03-27  0:48     ` [PATCH v3 16/18] unpack-trees: provide warnings on sparse updates for unmerged paths too Elijah Newren via GitGitGadget
2020-03-27  0:49     ` Elijah Newren via GitGitGadget [this message]
2020-03-27  0:49     ` [PATCH v3 18/18] sparse-checkout: provide a new reapply subcommand Elijah Newren via GitGitGadget
2020-03-27 13:22     ` [PATCH v3 00/18] Sparse checkout improvements -- improved sparsity updating Derrick Stolee

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=844306c3e86ef67591cc086decb2b760e7d710a3.1585270142.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git