git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / 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 5/7] unpack-trees: add a new update_sparsity() function
Date: Sat, 14 Mar 2020 07:11:31 +0000	[thread overview]
Message-ID: <9f13a2e8c64d47a5b3a4a2a96451decaf548ca4e.1584169893.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.726.git.git.1584169893.gitgitgadget@gmail.com>

From: Elijah Newren <newren@gmail.com>

Previously, the only way to update the SKIP_WORKTREE bits for various
paths was invoking `git read-tree -mu HEAD` or calling the same code
that this codepath invoked.  This however had a number of problems.
Let's look at all the various cases:

  Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files):
    A) There is no file in the way
    B) There is an untracked file in the way
  Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files):
    C) The path is clean
    D) The path is unmerged
    E) The path has unstaged changes
    F) The path has staged changes (differs from HEAD)

For `git read-tree -mu HEAD`, the behavior is as follows:
    A) Materialize the file, as expected
    B) Refuse to make *any* changes (to this path or any others)
    C) Remove the file, as expected
    D) Refuse to make *any* changes (to this path or any others)
    E) Refuse to make *any* changes (to this path or any others)
    F) Removes the file from the working copy AND resets the index
       to match HEAD

Refusing to modify files that could result in data loss as in cases B,
D, and E is good, but refusing to work on any other paths is very
problematic for users.  If they are in the middle of a rebase or have
made modifications to files that bring in more dependencies, then for
their build to work they need to update the sparse paths -- which they
are prevented from doing.  Sometimes they stage the files and re-try,
at which point they run into case F and believe that their changes are
lost.  (Even if they come ask an expert, they have to trawl through
their loose objects looking for which ones match which files.)

Add a new update_sparsity() function which behaves as follows in these
cases:
    A) Materialize the file, as expected
    B) Leave the file in the working copy alone, clear the SKIP_WORKTREE
       bit, and print a warning (thus leaving them in a state where
       status will report the file as modified, which seems logical).
    C) Remove the file, as expected
    D) Do NOT mark this path as SKIP_WORKTREE, but allow others paths
       to be updated.
    E) Do NOT mark this path as SKIP_WORKTREE and print a warning about
       the dirty path, but allow other paths to be updated.
    F) Mark the path as SKIP_WORKTREE, but do not revert the version
       stored in the index to match HEAD; leave the contents alone.

I tried a different behavior for B (leave the SKIP_WORKTREE bit set),
but found it very surprising and counter-intuitive (e.g. the user sees
it is present along with all the other files in that directory, tries to
stage it, but git add ignores it since the SKIP_WORKTREE bit is set).
A, B, C, and E all seem like optimal behavior to me.  D may be as well,
though I wonder if printing a warning would be an improvement.  Some
might be slightly surprised by F at first, but given that it does the
right thing with `git commit` and even `git commit -a` (add ignores
entries that are marked SKIP_WORKTREE and thus doesn't delete them, and
commit -a is similar), it seems logical to me.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 unpack-trees.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.h |   2 +
 2 files changed, 108 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index 8f10ac91ce1..21234635573 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -255,6 +255,25 @@ static void display_error_msgs(struct unpack_trees_options *o)
 		fprintf(stderr, _("Aborting\n"));
 }
 
+/*
+ * display all the "error" messages as warnings
+ */
+static void display_warning_msgs(struct unpack_trees_options *o)
+{
+	int e, i;
+	for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) {
+		struct string_list *rejects = &o->unpack_rejects[e];
+		if (rejects->nr > 0) {
+			struct strbuf path = STRBUF_INIT;
+			for (i = 0; i < rejects->nr; i++)
+				strbuf_addstr(&path, rejects->items[i].string);
+			warning(ERRORMSG(o, e), super_prefixed(path.buf));
+			strbuf_release(&path);
+		}
+		string_list_clear(rejects, 0);
+	}
+}
+
 static int check_submodule_move_head(const struct cache_entry *ce,
 				     const char *old_id,
 				     const char *new_id,
@@ -1708,6 +1727,93 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	goto done;
 }
 
+/*
+ * Update SKIP_WORKTREE bits according to sparsity patterns, and update
+ * working directory to match.
+ *
+ * Returns
+ *   0: success with no warnings
+ *   1: success with warnings (warnings come either from (a) dirty changes
+ *           present in which case we avoid marking those paths as
+ *           SKIP_WORKTREE, or (b) from untracked files being in the way us
+ *           checking a file out of the index, in which case we leave the file
+ *           in the working tree alone while clearing SKIP_WORKTREE)
+ *   -1: failure to manipulate the resulting index
+ *   -2: failure to reflect the changes to the work tree.
+ *
+ * CE_NEW_SKIP_WORKTREE is used internally.
+ */
+int update_sparsity(struct unpack_trees_options *o)
+{
+	struct pattern_list pl;
+	int i, empty_worktree, ret = 0;
+	unsigned old_show_all_errors;
+
+	old_show_all_errors = o->show_all_errors;
+	o->show_all_errors = 1;
+
+	/* Sanity checks */
+	if (!o->update || o->index_only || o->skip_sparse_checkout)
+		BUG("update_sparsity() is for reflecting sparsity patterns in working directory");
+	if (o->src_index != o->dst_index || o->fn)
+		BUG("update_sparsity() called wrong");
+
+	trace_performance_enter();
+
+	if (!o->pl) {
+		char *sparse = git_pathdup("info/sparse-checkout");
+		memset(&pl, 0, sizeof(pl));
+		pl.use_cone_patterns = core_sparse_checkout_cone;
+		if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0) {
+			/* FIXME: Skip to check_updates()?? */
+			o->skip_sparse_checkout = 1;
+			goto skip_sparse_checkout;
+		} else
+			o->pl = &pl;
+		free(sparse);
+	}
+
+	/* Set NEW_SKIP_WORKTREE on existing entries. */
+	mark_all_ce_unused(o->src_index);
+	mark_new_skip_worktree(o->pl, o->src_index, 0,
+			       CE_NEW_SKIP_WORKTREE, o->verbose_update);
+
+	/* Then loop over entries and update/remove as needed */
+	ret = 0;
+	empty_worktree = 1;
+	for (i = 0; i < o->src_index->cache_nr; i++) {
+		struct cache_entry *ce = o->src_index->cache[i];
+
+		if (apply_sparse_checkout(o->src_index, ce, o))
+			ret = 1;
+
+		if (!ce_skip_worktree(ce))
+			empty_worktree = 0;
+
+	}
+
+	/*
+	 * Sparse checkout is meant to narrow down checkout area
+	 * but it does not make sense to narrow down to empty working
+	 * tree. This is usually a mistake in sparse checkout rules.
+	 * Do not allow users to do that.
+	 */
+	if (o->src_index->cache_nr && empty_worktree) {
+		ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
+		goto done;
+	}
+
+skip_sparse_checkout:
+	if (check_updates(o, o->src_index))
+		ret = -2;
+
+done:
+	display_warning_msgs(o);
+	o->show_all_errors = old_show_all_errors;
+	trace_performance_leave("update_sparsity");
+	return ret;
+}
+
 /* Here come the merge functions */
 
 static int reject_merge(const struct cache_entry *ce,
diff --git a/unpack-trees.h b/unpack-trees.h
index ae1557fb804..5d1a77ae249 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -90,6 +90,8 @@ struct unpack_trees_options {
 int unpack_trees(unsigned n, struct tree_desc *t,
 		 struct unpack_trees_options *options);
 
+int update_sparsity(struct unpack_trees_options *options);
+
 int verify_uptodate(const struct cache_entry *ce,
 		    struct unpack_trees_options *o);
 
-- 
gitgitgadget


  parent reply	other threads:[~2020-03-15  3:46 UTC|newest]

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 ` Elijah Newren via GitGitGadget [this message]
2020-03-15 18:17   ` [PATCH 5/7] unpack-trees: add a new update_sparsity() function 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     ` [PATCH v3 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning Elijah Newren via GitGitGadget
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=9f13a2e8c64d47a5b3a4a2a96451decaf548ca4e.1584169893.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
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

	https://80x24.org/mirrors/git.git

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