* [PATCH v2 01/18] unpack-trees: fix minor typo in comment
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
@ 2020-03-21 17:59 ` Elijah Newren via GitGitGadget
2020-03-21 17:59 ` [PATCH v2 02/18] unpack-trees: remove unused error type Elijah Newren via GitGitGadget
` (18 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 1ecdab33040..0d0eec0221e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1618,7 +1618,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
/*
* Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1
- * If the will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE
+ * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE
* so apply_sparse_checkout() won't attempt to remove it from worktree
*/
mark_new_skip_worktree(o->pl, &o->result,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 02/18] unpack-trees: remove unused error type
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 ` Elijah Newren via GitGitGadget
2020-03-21 17:59 ` [PATCH v2 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
` (17 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
commit 08402b0409 ("merge-recursive: distinguish "removed" and
"overwritten" messages", 2010-08-11) split
ERROR_WOULD_LOSE_UNTRACKED
into both
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN
ERROR_WOULD_LOSE_UNTRACKED_REMOVED
and also split
ERROR_WOULD_LOSE_ORPHANED
into both
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN
ERROR_WOULD_LOSE_ORPHANED_REMOVED
However, despite the split only three of these four types were used.
ERROR_WOULD_LOSE_ORPHANED_REMOVED was not put into use when it was
introduced and nothing else has used it in the intervening decade
either. Remove it.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 4 ----
unpack-trees.h | 1 -
2 files changed, 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 0d0eec0221e..f72a7a21f9c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -49,8 +49,6 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
- /* ERROR_WOULD_LOSE_ORPHANED_REMOVED */
- "Working tree file '%s' would be removed by sparse checkout update.",
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
@@ -172,8 +170,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
- msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
- _("The following working tree files would be removed by sparse checkout update:\n%s");
msgs[ERROR_WOULD_LOSE_SUBMODULE] =
_("Cannot update submodule:\n%s");
diff --git a/unpack-trees.h b/unpack-trees.h
index ae1557fb804..6d7c7b6c2e0 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -24,7 +24,6 @@ enum unpack_trees_error_types {
ERROR_BIND_OVERLAP,
ERROR_SPARSE_NOT_UPTODATE_FILE,
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- ERROR_WOULD_LOSE_ORPHANED_REMOVED,
ERROR_WOULD_LOSE_SUBMODULE,
NB_UNPACK_TREES_ERROR_TYPES
};
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 03/18] unpack-trees: simplify verify_absent_sparse()
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 ` Elijah Newren via GitGitGadget
2020-03-21 17:59 ` [PATCH v2 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
` (16 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
verify_absent_sparse() was introduced in commit 08402b0409
("merge-recursive: distinguish "removed" and "overwritten" messages",
2010-08-11), and has always had exactly one caller which always passes
error_type == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN. This function
then checks whether error_type is this value, and if so, sets it instead
to ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN. It has been nearly a decade
and no other caller has been created, and no other value has ever been
passed, so just pass the expected value to begin with.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f72a7a21f9c..3af2e126abf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -506,7 +506,7 @@ static int apply_sparse_checkout(struct index_state *istate,
ce->ce_flags &= ~CE_UPDATE;
}
if (was_skip_worktree && !ce_skip_worktree(ce)) {
- if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
+ if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o))
return -1;
ce->ce_flags |= CE_UPDATE;
}
@@ -2026,11 +2026,7 @@ static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types error_type,
struct unpack_trees_options *o)
{
- enum unpack_trees_error_types orphaned_error = error_type;
- if (orphaned_error == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN)
- orphaned_error = ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN;
-
- return verify_absent_1(ce, orphaned_error, o);
+ return verify_absent_1(ce, error_type, o);
}
static int merged_entry(const struct cache_entry *ce,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 04/18] unpack-trees: simplify pattern_list freeing
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2020-03-21 17:59 ` [PATCH v2 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
@ 2020-03-21 17:59 ` 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
` (15 subsequent siblings)
19 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
commit e091228e17 ("sparse-checkout: update working directory
in-process", 2019-11-21) allowed passing a pre-defined set of patterns
to unpack_trees(). However, if o->pl was NULL, it would still read the
existing patterns and use those. If those patterns were read into a
data structure that was allocated, naturally they needed to be free'd.
However, despite the same function being responsible for knowing about
both the allocation and the free'ing, the logic for tracking whether to
free the pattern_list was hoisted to an outer function with an
additional flag in unpack_trees_options. Put the logic back in the
relevant function and discard the now unnecessary flag.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 1 -
unpack-trees.c | 6 ++++--
unpack-trees.h | 3 +--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 740da4b6d54..d102a9697fd 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl)
o.dst_index = r->index;
o.skip_sparse_checkout = 0;
o.pl = pl;
- o.keep_pattern_list = !!pl;
resolve_undo_clear_index(r->index);
setup_work_tree();
diff --git a/unpack-trees.c b/unpack-trees.c
index 3af2e126abf..d2863fa0310 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct pattern_list pl;
+ int free_pattern_list = 0;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
@@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
else
o->pl = &pl;
free(sparse);
+ free_pattern_list = 1;
}
memset(&o->result, 0, sizeof(o->result));
@@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
o->src_index = NULL;
done:
- trace_performance_leave("unpack_trees");
- if (!o->keep_pattern_list)
+ if (free_pattern_list)
clear_pattern_list(&pl);
+ trace_performance_leave("unpack_trees");
return ret;
return_failed:
diff --git a/unpack-trees.h b/unpack-trees.h
index 6d7c7b6c2e0..d3516267f36 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -58,8 +58,7 @@ struct unpack_trees_options {
quiet,
exiting_early,
show_all_errors,
- dry_run,
- keep_pattern_list;
+ dry_run;
const char *prefix;
int cache_bottom;
struct dir_struct *dir;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 04/18] unpack-trees: simplify pattern_list freeing
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
0 siblings, 0 replies; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 15:57 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 1:59 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> commit e091228e17 ("sparse-checkout: update working directory
> in-process", 2019-11-21) allowed passing a pre-defined set of patterns
> to unpack_trees(). However, if o->pl was NULL, it would still read the
> existing patterns and use those. If those patterns were read into a
> data structure that was allocated, naturally they needed to be free'd.
> However, despite the same function being responsible for knowing about
> both the allocation and the free'ing, the logic for tracking whether to
> free the pattern_list was hoisted to an outer function with an
> additional flag in unpack_trees_options. Put the logic back in the
> relevant function and discard the now unnecessary flag.
Your approach here is much cleaner. Thanks!
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> builtin/sparse-checkout.c | 1 -
> unpack-trees.c | 6 ++++--
> unpack-trees.h | 3 +--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 740da4b6d54..d102a9697fd 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl)
> o.dst_index = r->index;
> o.skip_sparse_checkout = 0;
> o.pl = pl;
> - o.keep_pattern_list = !!pl;
>
> resolve_undo_clear_index(r->index);
> setup_work_tree();
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3af2e126abf..d2863fa0310 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> int i, ret;
> static struct cache_entry *dfc;
> struct pattern_list pl;
> + int free_pattern_list = 0;
>
> if (len > MAX_UNPACK_TREES)
> die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> @@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> else
> o->pl = &pl;
> free(sparse);
> + free_pattern_list = 1;
> }
>
> memset(&o->result, 0, sizeof(o->result));
> @@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> o->src_index = NULL;
>
> done:
> - trace_performance_leave("unpack_trees");
> - if (!o->keep_pattern_list)
> + if (free_pattern_list)
> clear_pattern_list(&pl);
> + trace_performance_leave("unpack_trees");
> return ret;
>
> return_failed:
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 6d7c7b6c2e0..d3516267f36 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -58,8 +58,7 @@ struct unpack_trees_options {
> quiet,
> exiting_early,
> show_all_errors,
> - dry_run,
> - keep_pattern_list;
> + dry_run;
> const char *prefix;
> int cache_bottom;
> struct dir_struct *dir;
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 05/18] t1091: make some tests a little more defensive against failures
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (3 preceding siblings ...)
2020-03-21 17:59 ` [PATCH v2 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
@ 2020-03-21 17:59 ` 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
` (14 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 44a91205d60..8607a8e6d1a 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -278,6 +278,7 @@ test_expect_success 'cone mode: add parent path' '
'
test_expect_success 'revert to old sparse-checkout on bad update' '
+ test_when_finished git -C repo sparse-checkout disable &&
test_when_finished git -C repo reset --hard &&
git -C repo sparse-checkout set deep &&
echo update >repo/deep/deeper2/a &&
@@ -328,6 +329,7 @@ test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status'
'
test_expect_success 'cone mode: set with core.ignoreCase=true' '
+ rm repo/.git/info/sparse-checkout &&
git -C repo sparse-checkout init --cone &&
git -C repo -c core.ignoreCase=true sparse-checkout set folder1 &&
cat >expect <<-\EOF &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 06/18] unpack-trees: allow check_updates() to work on a different index
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (4 preceding siblings ...)
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 ` 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
` (13 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
check_updates() previously assumed it was working on o->result. We want
to use this function in combination with a different index_state, so
take the intended index_state as a parameter.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index d2863fa0310..dde50047a82 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -353,12 +353,12 @@ static void report_collided_checkout(struct index_state *index)
string_list_clear(&list, 0);
}
-static int check_updates(struct unpack_trees_options *o)
+static int check_updates(struct unpack_trees_options *o,
+ struct index_state *index)
{
unsigned cnt = 0;
int errs = 0;
struct progress *progress;
- struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
int i;
@@ -1665,7 +1665,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
}
- ret = check_updates(o) ? (-2) : 0;
+ ret = check_updates(o, &o->result) ? (-2) : 0;
if (o->dst_index) {
move_index_extensions(&o->result, o->src_index);
if (!ret) {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 07/18] unpack-trees: do not mark a dirty path with SKIP_WORKTREE
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (5 preceding siblings ...)
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 ` 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
` (12 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
If a path is dirty, removing from the working tree risks losing data.
As such, we want to make sure any such path is not marked with
SKIP_WORKTREE. While the current callers of this code detect this case
and re-populate with a previous set of sparsity patterns, we want to
allow some paths to be marked with SKIP_WORKTREE while others are left
unmarked without it being considered an error. The reason this
shouldn't be considered an error is that SKIP_WORKTREE has always been
an advisory-only setting; merge and rebase for example were free to
materialize paths and clear the SKIP_WORKTREE bit in order to accomplish
their work even though they kept the SKIP_WORKTREE bit set for other
paths. Leaving dirty working files in the working tree is thus a
natural extension of what we have already been doing.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index dde50047a82..e8e794880ab 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -500,8 +500,11 @@ static int apply_sparse_checkout(struct index_state *istate,
* also stat info may have lost after merged_entry() so calling
* verify_uptodate() again may fail
*/
- if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
+ if (!(ce->ce_flags & CE_UPDATE) &&
+ verify_uptodate_sparse(ce, o)) {
+ ce->ce_flags &= ~CE_SKIP_WORKTREE;
return -1;
+ }
ce->ce_flags |= CE_WT_REMOVE;
ce->ce_flags &= ~CE_UPDATE;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 08/18] unpack-trees: pull sparse-checkout pattern reading into a new function
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (6 preceding siblings ...)
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 ` 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
` (11 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 17:59 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Create a populate_from_existing_patterns() function for reading the
path_patterns from $GIT_DIR/info/sparse-checkout so that we can re-use
it elsewhere.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index e8e794880ab..4733e7eaf89 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1492,6 +1492,20 @@ static void mark_new_skip_worktree(struct pattern_list *pl,
clear_ce_flags(istate, select_flag, skip_wt_flag, pl, show_progress);
}
+static void populate_from_existing_patterns(struct unpack_trees_options *o,
+ struct pattern_list *pl)
+{
+ char *sparse = git_pathdup("info/sparse-checkout");
+
+ pl->use_cone_patterns = core_sparse_checkout_cone;
+ if (add_patterns_from_file_to_list(sparse, "", 0, pl, NULL) < 0)
+ o->skip_sparse_checkout = 1;
+ else
+ o->pl = pl;
+ free(sparse);
+}
+
+
static int verify_absent(const struct cache_entry *,
enum unpack_trees_error_types,
struct unpack_trees_options *);
@@ -1512,18 +1526,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
trace_performance_enter();
- memset(&pl, 0, sizeof(pl));
if (!core_apply_sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout && !o->pl) {
- char *sparse = git_pathdup("info/sparse-checkout");
- pl.use_cone_patterns = core_sparse_checkout_cone;
- if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0)
- o->skip_sparse_checkout = 1;
- else
- o->pl = &pl;
- free(sparse);
+ memset(&pl, 0, sizeof(pl));
free_pattern_list = 1;
+ populate_from_existing_patterns(o, &pl);
}
memset(&o->result, 0, sizeof(o->result));
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (7 preceding siblings ...)
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 ` Elijah Newren via GitGitGadget
2020-03-23 18:02 ` Derrick Stolee
2020-03-21 18:00 ` [PATCH v2 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
` (10 subsequent siblings)
19 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
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 if
the index or working directory were not clean. First, let's consider
the case:
Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files)
If the working tree was clean this was fine, but if there were files or
directories or symlinks or whatever already present at the given path
then the operation would abort with an error. Let's label this case
for later discussion:
A) There is an untracked path in the way
Now let's consider the opposite case:
Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files)
If the index and working tree was clean this was fine, but if there were
any unclean paths we would run into problems. There are three different
cases to consider:
B) The path is unmerged
C) The path has unstaged changes
D) The path has staged changes (differs from HEAD)
If any path fell into case B or C, then the whole operation would be
aborted with an error. With sparse-checkout, the whole operation would
be aborted for case D as well, but for its predecessor of using `git
read-tree -mu HEAD` directly, any paths that fell into case D would be
removed from the working copy and the index entry for that path would be
reset to match HEAD -- which looks and feels like data loss to users
(only a few are even aware to ask whether it can be recovered, and even
then it requires walking through loose objects trying to match up the
right ones).
Refusing to remove files that have unsaved user changes is good, but
refusing to work on any other paths is very problematic for users. If
the user is in the middle of a rebase or has made modifications to files
that bring in more dependencies, then for their build to work they need
to update the sparse paths. This logic has been preventing them from
doing so. Sometimes in response, the user will stage the files and
re-try, to no avail with sparse-checkout or to the horror of losing
their changes if they are using its predecessor of `git read-tree -mu
HEAD`.
Add a new update_sparsity() function which will not error out in any of
these cases but behaves as follows for the special cases:
A) Leave the file in the working copy alone, clear the SKIP_WORKTREE
bit, and print a warning (thus leaving the path in a state where
status will report the file as modified, which seems logical).
B) Do NOT mark this path as SKIP_WORKTREE, and leave it as unmerged.
C) Do NOT mark this path as SKIP_WORKTREE and print a warning about
the dirty path.
D) 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 A (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
& C seem like optimal behavior to me. B may be as well, though I wonder
if printing a warning would be an improvement. Some might be slightly
surprised by D at first, but given that it does the right thing with
`git commit` and even `git commit -a` (`git 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 | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
unpack-trees.h | 9 ++++++
2 files changed, 87 insertions(+)
diff --git a/unpack-trees.c b/unpack-trees.c
index 4733e7eaf89..6abea555929 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1714,6 +1714,84 @@ 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.
+ *
+ * CE_NEW_SKIP_WORKTREE is used internally.
+ */
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
+{
+ enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
+ struct pattern_list pl;
+ int i, empty_worktree;
+ unsigned old_show_all_errors;
+ int free_pattern_list = 0;
+
+ 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 we weren't given patterns, use the recorded ones */
+ if (!o->pl) {
+ memset(&pl, 0, sizeof(pl));
+ free_pattern_list = 1;
+ populate_from_existing_patterns(o, &pl);
+ if (o->skip_sparse_checkout)
+ goto skip_sparse_checkout;
+ }
+
+ /* 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 = UPDATE_SPARSITY_SUCCESS;
+ 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 = UPDATE_SPARSITY_WARNINGS;
+
+ 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) {
+ unpack_failed(o, "Sparse checkout leaves no entry on working directory");
+ ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
+ goto done;
+ }
+
+skip_sparse_checkout:
+ if (check_updates(o, o->src_index))
+ ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
+
+done:
+ display_error_msgs(o);
+ o->show_all_errors = old_show_all_errors;
+ if (free_pattern_list)
+ clear_pattern_list(&pl);
+ 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 d3516267f36..2c5d54cae9f 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -28,6 +28,13 @@ enum unpack_trees_error_types {
NB_UNPACK_TREES_ERROR_TYPES
};
+enum update_sparsity_result {
+ UPDATE_SPARSITY_SUCCESS = 0,
+ UPDATE_SPARSITY_WARNINGS = 1,
+ UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
+ UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
+};
+
/*
* Sets the list of user-friendly error messages to be used by the
* command "cmd" (either merge or checkout), and show_all_errors to 1.
@@ -88,6 +95,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
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function
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
0 siblings, 1 reply; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:02 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> 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 if
> the index or working directory were not clean. First, let's consider
> the case:
>
> Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files)
>
> If the working tree was clean this was fine, but if there were files or
> directories or symlinks or whatever already present at the given path
> then the operation would abort with an error. Let's label this case
> for later discussion:
>
> A) There is an untracked path in the way
>
> Now let's consider the opposite case:
>
> Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files)
>
> If the index and working tree was clean this was fine, but if there were
> any unclean paths we would run into problems. There are three different
> cases to consider:
>
> B) The path is unmerged
> C) The path has unstaged changes
> D) The path has staged changes (differs from HEAD)
>
> If any path fell into case B or C, then the whole operation would be
> aborted with an error. With sparse-checkout, the whole operation would
> be aborted for case D as well, but for its predecessor of using `git
> read-tree -mu HEAD` directly, any paths that fell into case D would be
> removed from the working copy and the index entry for that path would be
> reset to match HEAD -- which looks and feels like data loss to users
> (only a few are even aware to ask whether it can be recovered, and even
> then it requires walking through loose objects trying to match up the
> right ones).
>
> Refusing to remove files that have unsaved user changes is good, but
> refusing to work on any other paths is very problematic for users. If
> the user is in the middle of a rebase or has made modifications to files
> that bring in more dependencies, then for their build to work they need
> to update the sparse paths. This logic has been preventing them from
> doing so. Sometimes in response, the user will stage the files and
> re-try, to no avail with sparse-checkout or to the horror of losing
> their changes if they are using its predecessor of `git read-tree -mu
> HEAD`.
>
> Add a new update_sparsity() function which will not error out in any of
> these cases but behaves as follows for the special cases:
> A) Leave the file in the working copy alone, clear the SKIP_WORKTREE
> bit, and print a warning (thus leaving the path in a state where
> status will report the file as modified, which seems logical).
> B) Do NOT mark this path as SKIP_WORKTREE, and leave it as unmerged.
> C) Do NOT mark this path as SKIP_WORKTREE and print a warning about
> the dirty path.
> D) 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 A (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
> & C seem like optimal behavior to me. B may be as well, though I wonder
> if printing a warning would be an improvement. Some might be slightly
> surprised by D at first, but given that it does the right thing with
> `git commit` and even `git commit -a` (`git 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 | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
> unpack-trees.h | 9 ++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 4733e7eaf89..6abea555929 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1714,6 +1714,84 @@ 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.
> + *
> + * CE_NEW_SKIP_WORKTREE is used internally.
> + */
> +enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
> +{
> + enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
> + struct pattern_list pl;
> + int i, empty_worktree;
> + unsigned old_show_all_errors;
> + int free_pattern_list = 0;
> +
> + 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();
I was about to say "why didn't you use the trace2 regions like in
unpack_trees()?" when I discovered that we haven't sent them [1]
upstream yet. I'll put that on my TODO list.
[1] https://github.com/microsoft/git/commit/9a04644e14fe4aeb556dfc30cb2220b799f53448
> + /* If we weren't given patterns, use the recorded ones */
> + if (!o->pl) {
> + memset(&pl, 0, sizeof(pl));
> + free_pattern_list = 1;
I notice you are using the same free_pattern_list pattern as your
earlier commit. Good.
> + populate_from_existing_patterns(o, &pl);
> + if (o->skip_sparse_checkout)
> + goto skip_sparse_checkout;
> + }
> +
> + /* 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 = UPDATE_SPARSITY_SUCCESS;
> + 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 = UPDATE_SPARSITY_WARNINGS;
> +
> + if (!ce_skip_worktree(ce))
> + empty_worktree = 0;
> +
nit: extra whitespace-only line
> + }
> +
> + /*
> + * 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) {
> + unpack_failed(o, "Sparse checkout leaves no entry on working directory");
> + ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
> + goto done;
> + }
> +
> +skip_sparse_checkout:
> + if (check_updates(o, o->src_index))
> + ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
> +
> +done:
> + display_error_msgs(o);
> + o->show_all_errors = old_show_all_errors;
> + if (free_pattern_list)
> + clear_pattern_list(&pl);
> + 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 d3516267f36..2c5d54cae9f 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -28,6 +28,13 @@ enum unpack_trees_error_types {
> NB_UNPACK_TREES_ERROR_TYPES
> };
>
> +enum update_sparsity_result {
> + UPDATE_SPARSITY_SUCCESS = 0,
> + UPDATE_SPARSITY_WARNINGS = 1,
> + UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
> + UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
> +};
> +
Is there a reason this isn't located just before
update_sparsity()?
> /*
> * Sets the list of user-friendly error messages to be used by the
> * command "cmd" (either merge or checkout), and show_all_errors to 1.
> @@ -88,6 +95,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);
> +
This appears to not use the enum as it should.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function
2020-03-23 18:02 ` Derrick Stolee
@ 2020-03-23 18:10 ` Elijah Newren
2020-03-23 18:21 ` Derrick Stolee
0 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren @ 2020-03-23 18:10 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee
On Mon, Mar 23, 2020 at 11:02 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> > + 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 = UPDATE_SPARSITY_WARNINGS;
> > +
> > + if (!ce_skip_worktree(ce))
> > + empty_worktree = 0;
> > +
>
> nit: extra whitespace-only line
Will clean it up.
> > + }
> > +
> > + /*
> > + * 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) {
> > + unpack_failed(o, "Sparse checkout leaves no entry on working directory");
> > + ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
> > + goto done;
> > + }
> > +
> > +skip_sparse_checkout:
> > + if (check_updates(o, o->src_index))
> > + ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
> > +
> > +done:
> > + display_error_msgs(o);
> > + o->show_all_errors = old_show_all_errors;
> > + if (free_pattern_list)
> > + clear_pattern_list(&pl);
> > + 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 d3516267f36..2c5d54cae9f 100644
> > --- a/unpack-trees.h
> > +++ b/unpack-trees.h
> > @@ -28,6 +28,13 @@ enum unpack_trees_error_types {
> > NB_UNPACK_TREES_ERROR_TYPES
> > };
> >
> > +enum update_sparsity_result {
> > + UPDATE_SPARSITY_SUCCESS = 0,
> > + UPDATE_SPARSITY_WARNINGS = 1,
> > + UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
> > + UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
> > +};
> > +
>
> Is there a reason this isn't located just before
> update_sparsity()?
You mean move it to unpack-trees.c? If I did that, the code in
sparse-checkout.c that uses two of these values would fail to compile.
> > /*
> > * Sets the list of user-friendly error messages to be used by the
> > * command "cmd" (either merge or checkout), and show_all_errors to 1.
> > @@ -88,6 +95,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);
> > +
>
> This appears to not use the enum as it should.
Whoops! Will fix. (Interesting that the compiler didn't flag any
kind of warning based on mismatch of declared function return types
for update_sparsity() in the .c and .h files...)
Thanks,
Elijah
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function
2020-03-23 18:10 ` Elijah Newren
@ 2020-03-23 18:21 ` Derrick Stolee
2020-03-23 20:24 ` Junio C Hamano
0 siblings, 1 reply; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:21 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee
On 3/23/2020 2:10 PM, Elijah Newren wrote:
> On Mon, Mar 23, 2020 at 11:02 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
>>> +enum update_sparsity_result {
>>> + UPDATE_SPARSITY_SUCCESS = 0,
>>> + UPDATE_SPARSITY_WARNINGS = 1,
>>> + UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
>>> + UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
>>> +};
>>> +
>>
>> Is there a reason this isn't located just before
>> update_sparsity()?
>
> You mean move it to unpack-trees.c? If I did that, the code in
> sparse-checkout.c that uses two of these values would fail to compile.
No, I just meant the function declaration below in unpack-trees.h.
>>> +int update_sparsity(struct unpack_trees_options *options);
>>> +
>>
>> This appears to not use the enum as it should.
>
> Whoops! Will fix. (Interesting that the compiler didn't flag any
> kind of warning based on mismatch of declared function return types
> for update_sparsity() in the .c and .h files...)
*shrug* enums are essentially decoration over an int, so I'm not
surprised it can happen.
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function
2020-03-23 18:21 ` Derrick Stolee
@ 2020-03-23 20:24 ` Junio C Hamano
0 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2020-03-23 20:24 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren, Elijah Newren via GitGitGadget, Git Mailing List,
Derrick Stolee
Derrick Stolee <stolee@gmail.com> writes:
>>>> +int update_sparsity(struct unpack_trees_options *options);
>>>> +
>>>
>>> This appears to not use the enum as it should.
>>
>> Whoops! Will fix. (Interesting that the compiler didn't flag any
>> kind of warning based on mismatch of declared function return types
>> for update_sparsity() in the .c and .h files...)
>
> *shrug* enums are essentially decoration over an int, so I'm not
> surprised it can happen.
;-)
Yeah, the value of preferring enum over preprocessor macros because
it helps debuggers to show symbolic constants, is oversold, I think.
Types of variables and functions' return values need to be set to
the enum or the benefit won't come to us X-<.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 10/18] sparse-checkout: use new update_sparsity() function
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (8 preceding siblings ...)
2020-03-21 18:00 ` [PATCH v2 09/18] unpack-trees: add a new update_sparsity() function Elijah Newren via GitGitGadget
@ 2020-03-21 18:00 ` Elijah Newren via GitGitGadget
2020-03-23 18:07 ` Derrick Stolee
2020-03-21 18:00 ` [PATCH v2 11/18] sparse-checkout: use improved unpack_trees porcelain messages Elijah Newren via GitGitGadget
` (9 subsequent siblings)
19 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout
codepaths for setting the SKIP_WORKTREE bits and instead use the new
update_sparsity() function.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 40 ++++++++----------------------
t/t1091-sparse-checkout-builtin.sh | 35 +++++++++++++++++++-------
2 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index d102a9697fd..a55c60d7594 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -94,49 +94,35 @@ static int sparse_checkout_list(int argc, const char **argv)
static int update_working_directory(struct pattern_list *pl)
{
- int result = 0;
+ enum update_sparsity_result result;
struct unpack_trees_options o;
struct lock_file lock_file = LOCK_INIT;
- struct object_id oid;
- struct tree *tree;
- struct tree_desc t;
struct repository *r = the_repository;
- if (repo_read_index_unmerged(r))
- die(_("you need to resolve your current index first"));
-
- if (get_oid("HEAD", &oid))
- return 0;
-
- tree = parse_tree_indirect(&oid);
- parse_tree(tree);
- init_tree_desc(&t, tree->buffer, tree->size);
-
memset(&o, 0, sizeof(o));
o.verbose_update = isatty(2);
- o.merge = 1;
o.update = 1;
- o.fn = oneway_merge;
o.head_idx = -1;
o.src_index = r->index;
o.dst_index = r->index;
o.skip_sparse_checkout = 0;
o.pl = pl;
- resolve_undo_clear_index(r->index);
setup_work_tree();
- cache_tree_free(&r->index->cache_tree);
-
repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
- core_apply_sparse_checkout = 1;
- result = unpack_trees(1, &t, &o);
+ result = update_sparsity(&o);
- if (!result) {
- prime_cache_tree(r, r->index, tree);
+ if (result == UPDATE_SPARSITY_WARNINGS)
+ /*
+ * We don't do any special handling of warnings from untracked
+ * files in the way or dirty entries that can't be removed.
+ */
+ result = UPDATE_SPARSITY_SUCCESS;
+ if (result == UPDATE_SPARSITY_SUCCESS)
write_locked_index(r->index, &lock_file, COMMIT_LOCK);
- } else
+ else
rollback_lock_file(&lock_file);
return result;
@@ -303,8 +289,6 @@ static int sparse_checkout_init(int argc, const char **argv)
};
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("initialize sparse-checkout"), NULL, 1, 0);
argc = parse_options(argc, argv, NULL,
builtin_sparse_checkout_init_options,
@@ -559,8 +543,6 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
};
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("set sparse-checkout patterns"), NULL, 1, 0);
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
@@ -576,8 +558,6 @@ static int sparse_checkout_disable(int argc, const char **argv)
struct strbuf match_all = STRBUF_INIT;
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("disable sparse-checkout"), NULL, 1, 0);
memset(&pl, 0, sizeof(pl));
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 8607a8e6d1a..86ae422ff5c 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
check_files repo a deep folder1
'
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
test_when_finished git -C repo sparse-checkout disable &&
test_when_finished git -C repo reset --hard &&
git -C repo sparse-checkout set deep &&
+
echo update >repo/deep/deeper2/a &&
cp repo/.git/info/sparse-checkout expect &&
- test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
- test_i18ngrep "cannot set sparse-checkout patterns" err &&
- test_cmp repo/.git/info/sparse-checkout expect &&
- check_files repo/deep a deeper1 deeper2
+ test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
+
+ git -C repo sparse-checkout set deep/deeper1 2>err &&
+
+ test_i18ngrep "Cannot update sparse checkout" err &&
+ test_cmp expect repo/.git/info/sparse-checkout &&
+ check_files repo/deep a deeper1 deeper2 &&
+ check_files repo/deep/deeper1 a deepest &&
+ check_files repo/deep/deeper1/deepest a &&
+ check_files repo/deep/deeper2 a
'
test_expect_success 'revert to old sparse-checkout on empty update' '
@@ -316,12 +323,22 @@ test_expect_success '.gitignore should not warn about cone mode' '
test_i18ngrep ! "disabling cone patterns" err
'
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
git clone repo dirty &&
echo dirty >dirty/folder1/a &&
- test_must_fail git -C dirty sparse-checkout init &&
- test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- test_must_fail git -C dirty sparse-checkout disable &&
+
+ git -C dirty sparse-checkout init 2>err &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+
+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+
+ git -C dirty sparse-checkout disable &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 10/18] sparse-checkout: use new update_sparsity() function
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
0 siblings, 1 reply; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:07 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout
> codepaths for setting the SKIP_WORKTREE bits and instead use the new
> update_sparsity() function.
>
[snip]
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 8607a8e6d1a..86ae422ff5c 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
> check_files repo a deep folder1
> '
>
> -test_expect_success 'revert to old sparse-checkout on bad update' '
> +test_expect_success 'not-up-to-date does not block rest of sparsification' '
> test_when_finished git -C repo sparse-checkout disable &&
> test_when_finished git -C repo reset --hard &&
> git -C repo sparse-checkout set deep &&
> +
> echo update >repo/deep/deeper2/a &&
> cp repo/.git/info/sparse-checkout expect &&
> - test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
> - test_i18ngrep "cannot set sparse-checkout patterns" err &&
> - test_cmp repo/.git/info/sparse-checkout expect &&
> - check_files repo/deep a deeper1 deeper2
> + test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
> +
> + git -C repo sparse-checkout set deep/deeper1 2>err &&
> +
> + test_i18ngrep "Cannot update sparse checkout" err &&
> + test_cmp expect repo/.git/info/sparse-checkout &&
> + check_files repo/deep a deeper1 deeper2 &&
> + check_files repo/deep/deeper1 a deepest &&
> + check_files repo/deep/deeper1/deepest a &&
> + check_files repo/deep/deeper2 a
> '
This demonstrates some of the value of using update_sparsity.
I expect some more specific tests are coming later in the
series.
> test_expect_success 'revert to old sparse-checkout on empty update' '
> @@ -316,12 +323,22 @@ test_expect_success '.gitignore should not warn about cone mode' '
> test_i18ngrep ! "disabling cone patterns" err
> '
>
> -test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
> +test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
> git clone repo dirty &&
> echo dirty >dirty/folder1/a &&
> - test_must_fail git -C dirty sparse-checkout init &&
> - test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
> - test_must_fail git -C dirty sparse-checkout disable &&
> +
> + git -C dirty sparse-checkout init 2>err &&
> + test_i18ngrep "error" err &&
> + test_i18ngrep "Cannot update sparse checkout" err &&
> +
> + git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
> + test_i18ngrep "error" err &&
> + test_i18ngrep "Cannot update sparse checkout" err &&
> +
> + git -C dirty sparse-checkout disable &&
> + test_i18ngrep "error" err &&
> + test_i18ngrep "Cannot update sparse checkout" err &&
> +
Should the "error" messages we grep for be more specific? Are they
the same as the "Cannot update sparse checkout"?
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 10/18] sparse-checkout: use new update_sparsity() function
2020-03-23 18:07 ` Derrick Stolee
@ 2020-03-23 18:14 ` Elijah Newren
2020-03-23 18:22 ` Derrick Stolee
0 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren @ 2020-03-23 18:14 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee
On Mon, Mar 23, 2020 at 11:07 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout
> > codepaths for setting the SKIP_WORKTREE bits and instead use the new
> > update_sparsity() function.
> >
> [snip]
> > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> > index 8607a8e6d1a..86ae422ff5c 100755
> > --- a/t/t1091-sparse-checkout-builtin.sh
> > +++ b/t/t1091-sparse-checkout-builtin.sh
> > @@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
> > check_files repo a deep folder1
> > '
> >
> > -test_expect_success 'revert to old sparse-checkout on bad update' '
> > +test_expect_success 'not-up-to-date does not block rest of sparsification' '
> > test_when_finished git -C repo sparse-checkout disable &&
> > test_when_finished git -C repo reset --hard &&
> > git -C repo sparse-checkout set deep &&
> > +
> > echo update >repo/deep/deeper2/a &&
> > cp repo/.git/info/sparse-checkout expect &&
> > - test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
> > - test_i18ngrep "cannot set sparse-checkout patterns" err &&
> > - test_cmp repo/.git/info/sparse-checkout expect &&
> > - check_files repo/deep a deeper1 deeper2
> > + test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
> > +
> > + git -C repo sparse-checkout set deep/deeper1 2>err &&
> > +
> > + test_i18ngrep "Cannot update sparse checkout" err &&
> > + test_cmp expect repo/.git/info/sparse-checkout &&
> > + check_files repo/deep a deeper1 deeper2 &&
> > + check_files repo/deep/deeper1 a deepest &&
> > + check_files repo/deep/deeper1/deepest a &&
> > + check_files repo/deep/deeper2 a
> > '
>
> This demonstrates some of the value of using update_sparsity.
> I expect some more specific tests are coming later in the
> series.
>
> > test_expect_success 'revert to old sparse-checkout on empty update' '
> > @@ -316,12 +323,22 @@ test_expect_success '.gitignore should not warn about cone mode' '
> > test_i18ngrep ! "disabling cone patterns" err
> > '
> >
> > -test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
> > +test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
> > git clone repo dirty &&
> > echo dirty >dirty/folder1/a &&
> > - test_must_fail git -C dirty sparse-checkout init &&
> > - test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
> > - test_must_fail git -C dirty sparse-checkout disable &&
> > +
> > + git -C dirty sparse-checkout init 2>err &&
> > + test_i18ngrep "error" err &&
> > + test_i18ngrep "Cannot update sparse checkout" err &&
> > +
> > + git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
> > + test_i18ngrep "error" err &&
> > + test_i18ngrep "Cannot update sparse checkout" err &&
> > +
> > + git -C dirty sparse-checkout disable &&
> > + test_i18ngrep "error" err &&
> > + test_i18ngrep "Cannot update sparse checkout" err &&
> > +
>
> Should the "error" messages we grep for be more specific? Are they
> the same as the "Cannot update sparse checkout"?
The two lines are checking for the same message, it's just that the
error message is split across multiple lines and thus requires two
greps. The error messages are actually a bit of a mess, particularly
when they are triggered for multiple paths. That's something that
subsequent commits will clean up.
I can add a note about this to the commit message.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 10/18] sparse-checkout: use new update_sparsity() function
2020-03-23 18:14 ` Elijah Newren
@ 2020-03-23 18:22 ` Derrick Stolee
0 siblings, 0 replies; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:22 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee
On 3/23/2020 2:14 PM, Elijah Newren wrote:
> On Mon, Mar 23, 2020 at 11:07 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> Should the "error" messages we grep for be more specific? Are they
>> the same as the "Cannot update sparse checkout"?
>
> The two lines are checking for the same message, it's just that the
> error message is split across multiple lines and thus requires two
> greps. The error messages are actually a bit of a mess, particularly
> when they are triggered for multiple paths. That's something that
> subsequent commits will clean up.
I see that now! The very next commit cleans this up nicely.
> I can add a note about this to the commit message.
Thanks!
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 11/18] sparse-checkout: use improved unpack_trees porcelain messages
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (9 preceding siblings ...)
2020-03-21 18:00 ` [PATCH v2 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
@ 2020-03-21 18:00 ` 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
` (8 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
setup_unpack_trees_porcelain() provides much improved error/warning
messages; instead of a message that assumes that there is only one path
with a given problem despite being used by code that intentionally is
grouping and showing errors together, it uses a message designed to be
used with groups of paths. For example, this transforms
error: Entry ' folder1/a
folder2/a
' not uptodate. Cannot update sparse checkout.
into
error: Cannot update sparse checkout: the following entries are not up to date:
folder1/a
folder2/a
In the past the suboptimal messages were never actually triggered
because we would error out if the working directory wasn't clean before
we even called unpack_trees(). The previous commit changed that,
though, so let's use the better error messages.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 2 ++
t/t1091-sparse-checkout-builtin.sh | 9 +++------
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a55c60d7594..aa81199f85d 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -112,7 +112,9 @@ static int update_working_directory(struct pattern_list *pl)
repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
+ setup_unpack_trees_porcelain(&o, "sparse-checkout");
result = update_sparsity(&o);
+ clear_unpack_trees_porcelain(&o);
if (result == UPDATE_SPARSITY_WARNINGS)
/*
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 86ae422ff5c..93c650ac038 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -328,16 +328,13 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout disable &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 12/18] unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (10 preceding siblings ...)
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 ` 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
` (7 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
A minor change, but we want to convert the sparse messages to warnings
and this allows us to group warnings and errors.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 12 ++++++------
unpack-trees.h | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 6abea555929..5c99d588dc3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -43,15 +43,14 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_BIND_OVERLAP */
"Entry '%s' overlaps with '%s'. Cannot bind.",
+ /* ERROR_WOULD_LOSE_SUBMODULE */
+ "Submodule '%s' cannot checkout new HEAD.",
+
/* ERROR_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
/* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
-
-
- /* ERROR_WOULD_LOSE_SUBMODULE */
- "Submodule '%s' cannot checkout new HEAD.",
};
#define ERRORMSG(o,type) \
@@ -166,12 +165,13 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
*/
msgs[ERROR_BIND_OVERLAP] = _("Entry '%s' overlaps with '%s'. Cannot bind.");
+ msgs[ERROR_WOULD_LOSE_SUBMODULE] =
+ _("Cannot update submodule:\n%s");
+
msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
- msgs[ERROR_WOULD_LOSE_SUBMODULE] =
- _("Cannot update submodule:\n%s");
opts->show_all_errors = 1;
/* rejected paths may not have a static buffer */
diff --git a/unpack-trees.h b/unpack-trees.h
index 2c5d54cae9f..a656bbf810b 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -22,9 +22,9 @@ enum unpack_trees_error_types {
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,
ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
ERROR_BIND_OVERLAP,
+ ERROR_WOULD_LOSE_SUBMODULE,
ERROR_SPARSE_NOT_UPTODATE_FILE,
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- ERROR_WOULD_LOSE_SUBMODULE,
NB_UNPACK_TREES_ERROR_TYPES
};
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 13/18] unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (11 preceding siblings ...)
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 ` 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
` (6 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
We want to treat issues with setting the SKIP_WORKTREE bit as a warning
rather than an error; rename the enum values to reflect this intent as
a simple step towards that goal.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 12 ++++++------
unpack-trees.h | 8 +++++---
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 5c99d588dc3..0554842580b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -46,10 +46,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
- /* ERROR_SPARSE_NOT_UPTODATE_FILE */
+ /* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
- /* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
+ /* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
};
@@ -168,9 +168,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
msgs[ERROR_WOULD_LOSE_SUBMODULE] =
_("Cannot update submodule:\n%s");
- msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
+ msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
- msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
+ msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
opts->show_all_errors = 1;
@@ -509,7 +509,7 @@ static int apply_sparse_checkout(struct index_state *istate,
ce->ce_flags &= ~CE_UPDATE;
}
if (was_skip_worktree && !ce_skip_worktree(ce)) {
- if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o))
+ if (verify_absent_sparse(ce, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, o))
return -1;
ce->ce_flags |= CE_UPDATE;
}
@@ -1876,7 +1876,7 @@ int verify_uptodate(const struct cache_entry *ce,
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
- return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);
+ return verify_uptodate_1(ce, o, WARNING_SPARSE_NOT_UPTODATE_FILE);
}
/*
diff --git a/unpack-trees.h b/unpack-trees.h
index a656bbf810b..3c6452fe9e5 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -23,9 +23,11 @@ enum unpack_trees_error_types {
ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
ERROR_BIND_OVERLAP,
ERROR_WOULD_LOSE_SUBMODULE,
- ERROR_SPARSE_NOT_UPTODATE_FILE,
- ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- NB_UNPACK_TREES_ERROR_TYPES
+
+ WARNING_SPARSE_NOT_UPTODATE_FILE,
+ WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
+
+ NB_UNPACK_TREES_ERROR_TYPES,
};
enum update_sparsity_result {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 14/18] unpack-trees: split display_error_msgs() into two
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (12 preceding siblings ...)
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 ` 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
` (5 subsequent siblings)
19 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
display_error_msgs() is never called to show messages of both ERROR_*
and WARNING_* types at the same time; it is instead called multiple
times, separately for each type. Since we want to display these types
differently, make two slightly different versions of this function.
A subsequent commit will further modify unpack_trees() and how it calls
the new display_warning_msgs().
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 6 ++--
unpack-trees.c | 50 +++++++++++++++++++++++++-----
unpack-trees.h | 8 +++--
3 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 93c650ac038..0d93d3983e0 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -328,13 +328,13 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout disable &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 0554842580b..9ee04992ac6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -24,7 +24,7 @@
* situation better. See how "git checkout" and "git merge" replaces
* them using setup_unpack_trees_porcelain(), for example.
*/
-static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
+static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
/* ERROR_WOULD_OVERWRITE */
"Entry '%s' would be overwritten by merge. Cannot merge.",
@@ -46,6 +46,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
+ /* NB_UNPACK_TREES_ERROR_TYPES; just a meta value */
+ "",
+
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
@@ -222,7 +225,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
/*
* Otherwise, insert in a list for future display by
- * display_error_msgs()
+ * display_(error|warning)_msgs()
*/
string_list_append(&o->unpack_rejects[e], path);
return -1;
@@ -233,13 +236,16 @@ static int add_rejected_path(struct unpack_trees_options *o,
*/
static void display_error_msgs(struct unpack_trees_options *o)
{
- int e, i;
- int something_displayed = 0;
+ int e;
+ unsigned error_displayed = 0;
for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) {
struct string_list *rejects = &o->unpack_rejects[e];
+
if (rejects->nr > 0) {
+ int i;
struct strbuf path = STRBUF_INIT;
- something_displayed = 1;
+
+ error_displayed = 1;
for (i = 0; i < rejects->nr; i++)
strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
error(ERRORMSG(o, e), super_prefixed(path.buf));
@@ -247,10 +253,36 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
string_list_clear(rejects, 0);
}
- if (something_displayed)
+ if (error_displayed)
fprintf(stderr, _("Aborting\n"));
}
+/*
+ * display all the warning messages stored in a nice way
+ */
+static void display_warning_msgs(struct unpack_trees_options *o)
+{
+ int e;
+ unsigned warning_displayed = 0;
+ for (e = NB_UNPACK_TREES_ERROR_TYPES+1;
+ e < NB_UNPACK_TREES_WARNING_TYPES; e++) {
+ struct string_list *rejects = &o->unpack_rejects[e];
+
+ if (rejects->nr > 0) {
+ int i;
+ struct strbuf path = STRBUF_INIT;
+
+ warning_displayed = 1;
+ for (i = 0; i < rejects->nr; i++)
+ strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
+ warning(ERRORMSG(o, e), super_prefixed(path.buf));
+ strbuf_release(&path);
+ }
+ string_list_clear(rejects, 0);
+ }
+ if (warning_displayed)
+ fprintf(stderr, _("After fixing the above paths, you may want to run `git sparse-checkout reapply`.\n"));
+}
static int check_submodule_move_head(const struct cache_entry *ce,
const char *old_id,
const char *new_id,
@@ -1705,8 +1737,10 @@ 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)
@@ -1784,7 +1818,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
done:
- display_error_msgs(o);
+ display_warning_msgs(o);
o->show_all_errors = old_show_all_errors;
if (free_pattern_list)
clear_pattern_list(&pl);
diff --git a/unpack-trees.h b/unpack-trees.h
index 3c6452fe9e5..d91c65ae453 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -24,10 +24,12 @@ enum unpack_trees_error_types {
ERROR_BIND_OVERLAP,
ERROR_WOULD_LOSE_SUBMODULE,
+ NB_UNPACK_TREES_ERROR_TYPES,
+
WARNING_SPARSE_NOT_UPTODATE_FILE,
WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
- NB_UNPACK_TREES_ERROR_TYPES,
+ NB_UNPACK_TREES_WARNING_TYPES,
};
enum update_sparsity_result {
@@ -73,13 +75,13 @@ struct unpack_trees_options {
struct dir_struct *dir;
struct pathspec *pathspec;
merge_fn_t fn;
- const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+ const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
struct argv_array msgs_to_free;
/*
* Store error messages in an array, each case
* corresponding to a error message type
*/
- struct string_list unpack_rejects[NB_UNPACK_TREES_ERROR_TYPES];
+ struct string_list unpack_rejects[NB_UNPACK_TREES_WARNING_TYPES];
int head_idx;
int merge_size;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 14/18] unpack-trees: split display_error_msgs() into two
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
0 siblings, 0 replies; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:32 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> display_error_msgs() is never called to show messages of both ERROR_*
> and WARNING_* types at the same time; it is instead called multiple
> times, separately for each type. Since we want to display these types
> differently, make two slightly different versions of this function.
>
> A subsequent commit will further modify unpack_trees() and how it calls
> the new display_warning_msgs().
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> t/t1091-sparse-checkout-builtin.sh | 6 ++--
> unpack-trees.c | 50 +++++++++++++++++++++++++-----
> unpack-trees.h | 8 +++--
> 3 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 93c650ac038..0d93d3983e0 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -328,13 +328,13 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
> echo dirty >dirty/folder1/a &&
>
> git -C dirty sparse-checkout init 2>err &&
> - test_i18ngrep "error.*Cannot update sparse checkout" err &&
> + test_i18ngrep "warning.*Cannot update sparse checkout" err &&
>
> git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
> - test_i18ngrep "error.*Cannot update sparse checkout" err &&
> + test_i18ngrep "warning.*Cannot update sparse checkout" err &&
>
> git -C dirty sparse-checkout disable &&
> - test_i18ngrep "error.*Cannot update sparse checkout" err &&
> + test_i18ngrep "warning.*Cannot update sparse checkout" err &&
I'm very happy this change is so clearly demonstrated by
a simple test change like this.
> git -C dirty reset --hard &&
> git -C dirty sparse-checkout init &&
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 0554842580b..9ee04992ac6 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -24,7 +24,7 @@
> * situation better. See how "git checkout" and "git merge" replaces
> * them using setup_unpack_trees_porcelain(), for example.
> */
> -static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
> +static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
> /* ERROR_WOULD_OVERWRITE */
> "Entry '%s' would be overwritten by merge. Cannot merge.",
>
> @@ -46,6 +46,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
> /* ERROR_WOULD_LOSE_SUBMODULE */
> "Submodule '%s' cannot checkout new HEAD.",
>
> + /* NB_UNPACK_TREES_ERROR_TYPES; just a meta value */
> + "",
> +
> /* WARNING_SPARSE_NOT_UPTODATE_FILE */
> "Entry '%s' not uptodate. Cannot update sparse checkout.",
>
> @@ -222,7 +225,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
>
> /*
> * Otherwise, insert in a list for future display by
> - * display_error_msgs()
> + * display_(error|warning)_msgs()
> */
> string_list_append(&o->unpack_rejects[e], path);
> return -1;
> @@ -233,13 +236,16 @@ static int add_rejected_path(struct unpack_trees_options *o,
> */
> static void display_error_msgs(struct unpack_trees_options *o)
> {
> - int e, i;
> - int something_displayed = 0;
> + int e;
> + unsigned error_displayed = 0;
> for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) {
> struct string_list *rejects = &o->unpack_rejects[e];
> +
> if (rejects->nr > 0) {
> + int i;
> struct strbuf path = STRBUF_INIT;
> - something_displayed = 1;
> +
> + error_displayed = 1;
> for (i = 0; i < rejects->nr; i++)
> strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
> error(ERRORMSG(o, e), super_prefixed(path.buf));
> @@ -247,10 +253,36 @@ static void display_error_msgs(struct unpack_trees_options *o)
> }
> string_list_clear(rejects, 0);
> }
> - if (something_displayed)
> + if (error_displayed)
> fprintf(stderr, _("Aborting\n"));
> }
>
> +/*
> + * display all the warning messages stored in a nice way
> + */
> +static void display_warning_msgs(struct unpack_trees_options *o)
> +{
> + int e;
> + unsigned warning_displayed = 0;
> + for (e = NB_UNPACK_TREES_ERROR_TYPES+1;
super-nit: spaces around '+'
> + e < NB_UNPACK_TREES_WARNING_TYPES; e++) {
> + struct string_list *rejects = &o->unpack_rejects[e];
> +
> + if (rejects->nr > 0) {
> + int i;
> + struct strbuf path = STRBUF_INIT;
> +
> + warning_displayed = 1;
> + for (i = 0; i < rejects->nr; i++)
> + strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
> + warning(ERRORMSG(o, e), super_prefixed(path.buf));
> + strbuf_release(&path);
> + }
> + string_list_clear(rejects, 0);
> + }
> + if (warning_displayed)
> + fprintf(stderr, _("After fixing the above paths, you may want to run `git sparse-checkout reapply`.\n"));
> +}
> static int check_submodule_move_head(const struct cache_entry *ce,
> const char *old_id,
> const char *new_id,
> @@ -1705,8 +1737,10 @@ 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)
> @@ -1784,7 +1818,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
> ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
>
> done:
> - display_error_msgs(o);
> + display_warning_msgs(o);
> o->show_all_errors = old_show_all_errors;
> if (free_pattern_list)
> clear_pattern_list(&pl);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 3c6452fe9e5..d91c65ae453 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -24,10 +24,12 @@ enum unpack_trees_error_types {
> ERROR_BIND_OVERLAP,
> ERROR_WOULD_LOSE_SUBMODULE,
>
> + NB_UNPACK_TREES_ERROR_TYPES,
> +
I think this is a very clever way to partition the enum.
> WARNING_SPARSE_NOT_UPTODATE_FILE,
> WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
>
> - NB_UNPACK_TREES_ERROR_TYPES,
> + NB_UNPACK_TREES_WARNING_TYPES,
> };
>
> enum update_sparsity_result {
> @@ -73,13 +75,13 @@ struct unpack_trees_options {
> struct dir_struct *dir;
> struct pathspec *pathspec;
> merge_fn_t fn;
> - const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
> + const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
> struct argv_array msgs_to_free;
> /*
> * Store error messages in an array, each case
> * corresponding to a error message type
> */
> - struct string_list unpack_rejects[NB_UNPACK_TREES_ERROR_TYPES];
> + struct string_list unpack_rejects[NB_UNPACK_TREES_WARNING_TYPES];
>
> int head_idx;
> int merge_size;
LGTM other than my really nitty nit-pick. Feel free to ignore.
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 15/18] unpack-trees: make sparse path messages sound like warnings
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (13 preceding siblings ...)
2020-03-21 18:00 ` [PATCH v2 14/18] unpack-trees: split display_error_msgs() into two Elijah Newren via GitGitGadget
@ 2020-03-21 18:00 ` 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
` (4 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
The messages for problems with sparse paths are phrased as errors that
cause the operation to abort, even though we are not making the
operation abort. Reword the messages to make sense in their new
context.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 8 ++++----
unpack-trees.c | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 0d93d3983e0..11eb567f3fb 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -288,7 +288,7 @@ test_expect_success 'not-up-to-date does not block rest of sparsification' '
git -C repo sparse-checkout set deep/deeper1 2>err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "The following paths are not up to date" err &&
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo/deep a deeper1 deeper2 &&
check_files repo/deep/deeper1 a deepest &&
@@ -328,13 +328,13 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
git -C dirty sparse-checkout disable &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 9ee04992ac6..7c24933016b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -50,10 +50,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
"",
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
- "Entry '%s' not uptodate. Cannot update sparse checkout.",
+ "Path '%s' not uptodate; will not remove from working tree.",
/* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
- "Working tree file '%s' would be overwritten by sparse checkout update.",
+ "Path '%s' already present; will not overwrite with sparse update.",
};
#define ERRORMSG(o,type) \
@@ -172,9 +172,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
_("Cannot update submodule:\n%s");
msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
- _("Cannot update sparse checkout: the following entries are not up to date:\n%s");
+ _("The following paths are not up to date and were left despite sparse patterns:\n%s");
msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
- _("The following working tree files would be overwritten by sparse checkout update:\n%s");
+ _("The following paths were already present and thus not updated despite sparse patterns:\n%s");
opts->show_all_errors = 1;
/* rejected paths may not have a static buffer */
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 16/18] unpack-trees: provide warnings on sparse updates for unmerged paths too
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (14 preceding siblings ...)
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 ` 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
` (3 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
When sparse-checkout runs to update the list of sparsity patterns, it
gives warnings if it can't remove paths from the working tree because
those files have dirty changes. Add a similar warning for unmerged
paths as well.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 24 ++++++++++++++++++++++++
unpack-trees.c | 30 ++++++++++++++++++++++++++++++
unpack-trees.h | 1 +
3 files changed, 55 insertions(+)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 11eb567f3fb..39093bcd5ec 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -342,6 +342,30 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
git -C dirty sparse-checkout disable
'
+test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged status' '
+ git clone repo unmerged &&
+
+ cat >input <<-EOF &&
+ 0 0000000000000000000000000000000000000000 folder1/a
+ 100644 $(git -C unmerged rev-parse HEAD:folder1/a) 1 folder1/a
+ EOF
+ git -C unmerged update-index --index-info <input &&
+
+ git -C unmerged sparse-checkout init 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged sparse-checkout disable &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged reset --hard &&
+ git -C unmerged sparse-checkout init &&
+ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
+ git -C unmerged sparse-checkout disable
+'
+
test_expect_success 'cone mode: set with core.ignoreCase=true' '
rm repo/.git/info/sparse-checkout &&
git -C repo sparse-checkout init --cone &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7c24933016b..298241a5e0e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Path '%s' not uptodate; will not remove from working tree.",
+ /* WARNING_SPARSE_UNMERGED_FILE */
+ "Path '%s' unmerged; will not remove from working tree.",
+
/* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
"Path '%s' already present; will not overwrite with sparse update.",
};
@@ -173,6 +176,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
_("The following paths are not up to date and were left despite sparse patterns:\n%s");
+ msgs[WARNING_SPARSE_UNMERGED_FILE] =
+ _("The following paths are unmerged and were left despite sparse patterns:\n%s");
msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
_("The following paths were already present and thus not updated despite sparse patterns:\n%s");
@@ -548,6 +553,23 @@ static int apply_sparse_checkout(struct index_state *istate,
return 0;
}
+static int warn_conflicted_path(struct index_state *istate,
+ int i,
+ struct unpack_trees_options *o)
+{
+ char *conflicting_path = istate->cache[i]->name;
+ int count = 0;
+
+ add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
+
+ /* Find out how many higher stage entries at same path */
+ while (++count < istate->cache_nr &&
+ !strcmp(conflicting_path,
+ istate->cache[i+count]->name))
+ /* do nothing */;
+ return count;
+}
+
static inline int call_unpack_fn(const struct cache_entry * const *src,
struct unpack_trees_options *o)
{
@@ -1793,6 +1815,14 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
for (i = 0; i < o->src_index->cache_nr; i++) {
struct cache_entry *ce = o->src_index->cache[i];
+
+ if (ce_stage(ce)) {
+ /* -1 because for loop will increment by 1 */
+ i += warn_conflicted_path(o->src_index, i, o) - 1;
+ ret = UPDATE_SPARSITY_WARNINGS;
+ continue;
+ }
+
if (apply_sparse_checkout(o->src_index, ce, o))
ret = UPDATE_SPARSITY_WARNINGS;
diff --git a/unpack-trees.h b/unpack-trees.h
index d91c65ae453..f970fd6c2f4 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -27,6 +27,7 @@ enum unpack_trees_error_types {
NB_UNPACK_TREES_ERROR_TYPES,
WARNING_SPARSE_NOT_UPTODATE_FILE,
+ WARNING_SPARSE_UNMERGED_FILE,
WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
NB_UNPACK_TREES_WARNING_TYPES,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (15 preceding siblings ...)
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 ` Elijah Newren via GitGitGadget
2020-03-21 18:00 ` [PATCH v2 18/18] sparse-checkout: provide a new reapply subcommand Elijah Newren via GitGitGadget
` (2 subsequent siblings)
19 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
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.
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 298241a5e0e..cab2177c951 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
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 18/18] sparse-checkout: provide a new reapply subcommand
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (16 preceding siblings ...)
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 ` 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-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
19 siblings, 1 reply; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-21 18:00 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
If commands like merge or rebase materialize files as part of their work,
or a previous sparse-checkout command failed to update individual files
due to dirty changes, users may want a command to simply 'reapply' the
sparsity rules. Provide one.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-sparse-checkout.txt | 10 ++++++++++
builtin/sparse-checkout.c | 10 +++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index c0342e53938..1a3ace60820 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -70,6 +70,16 @@ C-style quoted strings.
`core.sparseCheckoutCone` is enabled, the given patterns are interpreted
as directory names as in the 'set' subcommand.
+'reapply::
+ Reapply the sparsity pattern rules to paths in the working tree.
+ Commands like merge or rebase can materialize paths to do their
+ work (e.g. in order to show you a conflict), and other
+ sparse-checkout commands might fail to sparsify an individual file
+ (e.g. because it has unstaged changes or conflicts). In such
+ cases, it can make sense to run `git sparse-checkout reapply` later
+ after cleaning up affected paths (e.g. resolving conflicts, undoing
+ or committing changes, etc.).
+
'disable'::
Disable the `core.sparseCheckout` config setting, and restore the
working directory to include all files. Leaves the sparse-checkout
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index aa81199f85d..95d08824172 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -18,7 +18,7 @@
static const char *empty_base = "";
static char const * const builtin_sparse_checkout_usage[] = {
- N_("git sparse-checkout (init|list|set|add|disable) <options>"),
+ N_("git sparse-checkout (init|list|set|add|reapply|disable) <options>"),
NULL
};
@@ -554,6 +554,12 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
return modify_pattern_list(argc, argv, m);
}
+static int sparse_checkout_reapply(int argc, const char **argv)
+{
+ repo_read_index(the_repository);
+ return update_working_directory(NULL);
+}
+
static int sparse_checkout_disable(int argc, const char **argv)
{
struct pattern_list pl;
@@ -603,6 +609,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
return sparse_checkout_set(argc, argv, prefix, REPLACE);
if (!strcmp(argv[0], "add"))
return sparse_checkout_set(argc, argv, prefix, ADD);
+ if (!strcmp(argv[0], "reapply"))
+ return sparse_checkout_reapply(argc, argv);
if (!strcmp(argv[0], "disable"))
return sparse_checkout_disable(argc, argv);
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 18/18] sparse-checkout: provide a new reapply subcommand
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
0 siblings, 0 replies; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:40 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> If commands like merge or rebase materialize files as part of their work,
> or a previous sparse-checkout command failed to update individual files
> due to dirty changes, users may want a command to simply 'reapply' the
> sparsity rules. Provide one.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> Documentation/git-sparse-checkout.txt | 10 ++++++++++
> builtin/sparse-checkout.c | 10 +++++++++-
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index c0342e53938..1a3ace60820 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -70,6 +70,16 @@ C-style quoted strings.
> `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
> as directory names as in the 'set' subcommand.
>
> +'reapply::
> + Reapply the sparsity pattern rules to paths in the working tree.
> + Commands like merge or rebase can materialize paths to do their
> + work (e.g. in order to show you a conflict), and other
> + sparse-checkout commands might fail to sparsify an individual file
> + (e.g. because it has unstaged changes or conflicts). In such
> + cases, it can make sense to run `git sparse-checkout reapply` later
> + after cleaning up affected paths (e.g. resolving conflicts, undoing
> + or committing changes, etc.).
> +
> 'disable'::
> Disable the `core.sparseCheckout` config setting, and restore the
> working directory to include all files. Leaves the sparse-checkout
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index aa81199f85d..95d08824172 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -18,7 +18,7 @@
> static const char *empty_base = "";
>
> static char const * const builtin_sparse_checkout_usage[] = {
> - N_("git sparse-checkout (init|list|set|add|disable) <options>"),
> + N_("git sparse-checkout (init|list|set|add|reapply|disable) <options>"),
> NULL
> };
>
> @@ -554,6 +554,12 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
> return modify_pattern_list(argc, argv, m);
> }
>
> +static int sparse_checkout_reapply(int argc, const char **argv)
> +{
> + repo_read_index(the_repository);
> + return update_working_directory(NULL);
> +}
> +
> static int sparse_checkout_disable(int argc, const char **argv)
> {
> struct pattern_list pl;
> @@ -603,6 +609,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
> return sparse_checkout_set(argc, argv, prefix, REPLACE);
> if (!strcmp(argv[0], "add"))
> return sparse_checkout_set(argc, argv, prefix, ADD);
> + if (!strcmp(argv[0], "reapply"))
> + return sparse_checkout_reapply(argc, argv);
> if (!strcmp(argv[0], "disable"))
> return sparse_checkout_disable(argc, argv);
> }
I feel it necessary to commend you for how simple this patch is.
You did all the hard work in earlier patches to make this one
extremely simple.
Nice!
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/18] Sparse checkout improvements -- improved sparsity updating
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (17 preceding siblings ...)
2020-03-21 18:00 ` [PATCH v2 18/18] sparse-checkout: provide a new reapply subcommand Elijah Newren via GitGitGadget
@ 2020-03-23 18:41 ` Derrick Stolee
2020-03-23 20:26 ` Junio C Hamano
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
19 siblings, 1 reply; 69+ messages in thread
From: Derrick Stolee @ 2020-03-23 18:41 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/21/2020 1:59 PM, Elijah Newren via GitGitGadget wrote:
> This series provides a replacement for the sparsity updating in
> sparse-checkout that is based on the logic from git read-tree -mu HEAD. The
> most important bit is patch 9 and its lengthy commit message explaining the
> current state and rationale for most the series, though patches 16 and 17
> have additional related directions and rationale for the series. Those three
> patches are the most important to review.
>
> Changes since v1:
>
> * addressed several cleanups highlighted by Stolee (and I picked 'reapply'
> for the new subcommand name)
> * added three new minor cleanup commits (patches 2-4)
> * several new patches to adjust the warning messages to look like warnings
> (patches 11-15)
> * display warning messages when paths are unmerged (patch 16)
> * also make checkout and other unpack_trees()-using commands not error out
> when unable to set the SKIP_WORKTREE bit (patch 17)
I'm very happy with the updates in this version.
> Also, the loop range in display_warning_msgs() in patch 14 might seem
> slightly weird, but I felt adding another unpack_rejects array of some sort
> and plumbing through an extra bit everywhere to notify the system which
> unpack_rejects array to use would have been more invasive. Opinions either
> way (or alternative approaches) welcome.
>
> I was also curious if I should have squashed all the patches to adjust the
> warning messages (patches 11-15, they adjust the messages to actually look
> like warnings instead of errors), since they are individually kind of small,
> but it looked like it'd be much easier to review if I were to split them
> apart, so I did so.
I found them very easy to review. I had the one comment that was my fault
for not looking one patch ahead. In general, it was simple to see that you
were doing good updates with good motivations.
This is going to help the UX around sparse-checkout quite a bit!
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/18] Sparse checkout improvements -- improved sparsity updating
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
0 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2020-03-23 20:26 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee, Elijah Newren
Derrick Stolee <stolee@gmail.com> writes:
>> Changes since v1:
>>
>> * addressed several cleanups highlighted by Stolee (and I picked 'reapply'
>> for the new subcommand name)
>> * added three new minor cleanup commits (patches 2-4)
>> * several new patches to adjust the warning messages to look like warnings
>> (patches 11-15)
>> * display warning messages when paths are unmerged (patch 16)
>> * also make checkout and other unpack_trees()-using commands not error out
>> when unable to set the SKIP_WORKTREE bit (patch 17)
>
> I'm very happy with the updates in this version.
> ...
> This is going to help the UX around sparse-checkout quite a bit!
>
> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Thanks, both of you. Will queue.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 00/18] Sparse checkout improvements -- improved sparsity updating
2020-03-21 17:59 ` [PATCH v2 00/18] " Elijah Newren via GitGitGadget
` (18 preceding siblings ...)
2020-03-23 18:41 ` [PATCH v2 00/18] Sparse checkout improvements -- improved sparsity updating Derrick Stolee
@ 2020-03-27 0:48 ` Elijah Newren via GitGitGadget
2020-03-27 0:48 ` [PATCH v3 01/18] unpack-trees: fix minor typo in comment Elijah Newren via GitGitGadget
` (18 more replies)
19 siblings, 19 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren
This series provides a replacement for the sparsity updating in
sparse-checkout that is based on the logic from git read-tree -mu HEAD. The
most important bit is patch 9 and its lengthy commit message explaining the
current state and rationale for most the series, though patches 16 and 17
have additional related directions and rationale for the series. Those three
patches are the most important to review.
Changes since v2:
* addressed Stolee's cleanups and added his Reviewed-by to the series
(hopefully the next two changes don't invalidate that)
* added a test for the new 'reapply' subcommand (noticed it was missing
with Stolee's test coverage report)
* fix a minor issue with two of the other tests I modified -- when I'm
grepping through stderr for a command, I need to make sure to actually
record stderr from that command (otherwise my grep is looking through the
stderr of a previous command that was recorded). Also, since I messed
this up, I added one or two more sanity checks while I was at it.
Elijah Newren (18):
unpack-trees: fix minor typo in comment
unpack-trees: remove unused error type
unpack-trees: simplify verify_absent_sparse()
unpack-trees: simplify pattern_list freeing
t1091: make some tests a little more defensive against failures
unpack-trees: allow check_updates() to work on a different index
unpack-trees: do not mark a dirty path with SKIP_WORKTREE
unpack-trees: pull sparse-checkout pattern reading into a new function
unpack-trees: add a new update_sparsity() function
sparse-checkout: use new update_sparsity() function
sparse-checkout: use improved unpack_trees porcelain messages
unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
unpack-trees: split display_error_msgs() into two
unpack-trees: make sparse path messages sound like warnings
unpack-trees: provide warnings on sparse updates for unmerged paths
too
unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
sparse-checkout: provide a new reapply subcommand
Documentation/git-sparse-checkout.txt | 10 +
builtin/sparse-checkout.c | 55 +++---
t/t1011-read-tree-sparse-checkout.sh | 11 +-
t/t1091-sparse-checkout-builtin.sh | 105 ++++++++++-
t/t2018-checkout-branch.sh | 22 +++
unpack-trees.c | 255 ++++++++++++++++++++------
unpack-trees.h | 28 ++-
7 files changed, 375 insertions(+), 111 deletions(-)
base-commit: be8661a3286c67a5d4088f4226cbd7f8b76544b0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-726%2Fnewren%2Fsparse-checkout-improvements-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-726/newren/sparse-checkout-improvements-v3
Pull-Request: https://github.com/git/git/pull/726
Range-diff vs v2:
1: 3c183727c98 ! 1: bb41a2a52f4 unpack-trees: fix minor typo in comment
@@ -2,6 +2,7 @@
unpack-trees: fix minor typo in comment
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
2: e6beb1c5810 ! 2: 1dc36eb33f7 unpack-trees: remove unused error type
@@ -19,6 +19,7 @@
introduced and nothing else has used it in the intervening decade
either. Remove it.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
3: 739b52393fe ! 3: 97a95e91d8f unpack-trees: simplify verify_absent_sparse()
@@ -11,6 +11,7 @@
and no other caller has been created, and no other value has ever been
passed, so just pass the expected value to begin with.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
4: 65772dd46df ! 4: 1ce5dbe9c68 unpack-trees: simplify pattern_list freeing
@@ -13,6 +13,7 @@
additional flag in unpack_trees_options. Put the logic back in the
relevant function and discard the now unnecessary flag.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
5: 5cbab2a7a56 ! 5: da4b74093cd t1091: make some tests a little more defensive against failures
@@ -2,6 +2,7 @@
t1091: make some tests a little more defensive against failures
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
6: 5fea19f0136 ! 6: 555eedf5380 unpack-trees: allow check_updates() to work on a different index
@@ -6,6 +6,7 @@
to use this function in combination with a different index_state, so
take the intended index_state as a parameter.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
7: 3d2531ca818 ! 7: 10ec70fa697 unpack-trees: do not mark a dirty path with SKIP_WORKTREE
@@ -15,6 +15,7 @@
paths. Leaving dirty working files in the working tree is thus a
natural extension of what we have already been doing.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
8: 27d6304d33f ! 8: 8ed2f4e49ed unpack-trees: pull sparse-checkout pattern reading into a new function
@@ -6,6 +6,7 @@
path_patterns from $GIT_DIR/info/sparse-checkout so that we can re-use
it elsewhere.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
9: a46439c8536 ! 9: 3a1f084641e unpack-trees: add a new update_sparsity() function
@@ -71,6 +71,7 @@
are marked SKIP_WORKTREE and thus doesn't delete them, and `commit -a`
is similar), it seems logical to me.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
@@ -130,7 +131,6 @@
+
+ if (!ce_skip_worktree(ce))
+ empty_worktree = 0;
-+
+ }
+
+ /*
@@ -166,8 +166,8 @@
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@
- NB_UNPACK_TREES_ERROR_TYPES
- };
+ int unpack_trees(unsigned n, struct tree_desc *t,
+ struct unpack_trees_options *options);
+enum update_sparsity_result {
+ UPDATE_SPARSITY_SUCCESS = 0,
@@ -176,14 +176,7 @@
+ UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
+};
+
- /*
- * Sets the list of user-friendly error messages to be used by the
- * command "cmd" (either merge or checkout), and show_all_errors to 1.
-@@
- int unpack_trees(unsigned n, struct tree_desc *t,
- struct unpack_trees_options *options);
-
-+int update_sparsity(struct unpack_trees_options *options);
++enum update_sparsity_result update_sparsity(struct unpack_trees_options *options);
+
int verify_uptodate(const struct cache_entry *ce,
struct unpack_trees_options *o);
10: 27ed4a3f7a2 ! 10: 9040ac97175 sparse-checkout: use new update_sparsity() function
@@ -6,6 +6,12 @@
codepaths for setting the SKIP_WORKTREE bits and instead use the new
update_sparsity() function.
+ Note that when an issue is hit, the error message splits 'error' and
+ 'Cannot update sparse checkout' on separate lines. For now, we use two
+ greps to find both pieces of the error message but subsequent commits
+ will clean up the messages reported to the user.
+
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
@@ -147,14 +153,21 @@
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+
-+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
++ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
++ test_path_is_file dirty/folder1/a &&
+
-+ git -C dirty sparse-checkout disable &&
-+ test_i18ngrep "error" err &&
-+ test_i18ngrep "Cannot update sparse checkout" err &&
++ git -C dirty sparse-checkout disable 2>err &&
++ test_must_be_empty err &&
+
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+- git -C dirty sparse-checkout disable
++ test_path_is_missing dirty/folder1/a &&
++ git -C dirty sparse-checkout disable &&
++ test_path_is_file dirty/folder1/a
+ '
+
+ test_expect_success 'cone mode: set with core.ignoreCase=true' '
11: 79b9cc1df55 ! 11: e9c7e8ec46f sparse-checkout: use improved unpack_trees porcelain messages
@@ -23,6 +23,7 @@
we even called unpack_trees(). The previous commit changed that,
though, so let's use the better error messages.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
@@ -50,15 +51,10 @@
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
- git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_path_is_file dirty/folder1/a &&
- git -C dirty sparse-checkout disable &&
-- test_i18ngrep "error" err &&
-- test_i18ngrep "Cannot update sparse checkout" err &&
-+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
-
- git -C dirty reset --hard &&
- git -C dirty sparse-checkout init &&
+ git -C dirty sparse-checkout disable 2>err &&
12: 2ba7bbdaee3 ! 12: c3741107a9d unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
@@ -5,6 +5,7 @@
A minor change, but we want to convert the sparse messages to warnings
and this allows us to group warnings and errors.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
13: 203ece0ec2f ! 13: 90c514e220b unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
@@ -6,6 +6,7 @@
rather than an error; rename the enum values to reflect this intent as
a simple step towards that goal.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/unpack-trees.c b/unpack-trees.c
@@ -72,4 +73,4 @@
+ NB_UNPACK_TREES_ERROR_TYPES,
};
- enum update_sparsity_result {
+ /*
14: 57679c8e292 ! 14: 5676e890f65 unpack-trees: split display_error_msgs() into two
@@ -10,6 +10,7 @@
A subsequent commit will further modify unpack_trees() and how it calls
the new display_warning_msgs().
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
@@ -22,16 +23,12 @@
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
- git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_path_is_file dirty/folder1/a &&
- git -C dirty sparse-checkout disable &&
-- test_i18ngrep "error.*Cannot update sparse checkout" err &&
-+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
-
- git -C dirty reset --hard &&
- git -C dirty sparse-checkout init &&
+ git -C dirty sparse-checkout disable 2>err &&
diff --git a/unpack-trees.c b/unpack-trees.c
--- a/unpack-trees.c
@@ -100,7 +97,7 @@
+{
+ int e;
+ unsigned warning_displayed = 0;
-+ for (e = NB_UNPACK_TREES_ERROR_TYPES+1;
++ for (e = NB_UNPACK_TREES_ERROR_TYPES + 1;
+ e < NB_UNPACK_TREES_WARNING_TYPES; e++) {
+ struct string_list *rejects = &o->unpack_rejects[e];
+
@@ -160,7 +157,7 @@
+ NB_UNPACK_TREES_WARNING_TYPES,
};
- enum update_sparsity_result {
+ /*
@@
struct dir_struct *dir;
struct pathspec *pathspec;
15: 02a8e01f35b ! 15: 74e13ccee40 unpack-trees: make sparse path messages sound like warnings
@@ -7,6 +7,7 @@
operation abort. Reword the messages to make sense in their new
context.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
@@ -28,16 +29,12 @@
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
- git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
+ test_path_is_file dirty/folder1/a &&
- git -C dirty sparse-checkout disable &&
-- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
-+ test_i18ngrep "warning.*The following paths are not up to date" err &&
-
- git -C dirty reset --hard &&
- git -C dirty sparse-checkout init &&
+ git -C dirty sparse-checkout disable 2>err &&
diff --git a/unpack-trees.c b/unpack-trees.c
--- a/unpack-trees.c
16: f664a6dcbb3 ! 16: 5d9e8a9d27e unpack-trees: provide warnings on sparse updates for unmerged paths too
@@ -7,13 +7,14 @@
those files have dirty changes. Add a similar warning for unmerged
paths as well.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@
- git -C dirty sparse-checkout disable
+ test_path_is_file dirty/folder1/a
'
+test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged status' '
@@ -28,10 +29,11 @@
+ git -C unmerged sparse-checkout init 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
-+ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
++ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
++ test_path_is_file dirty/folder1/a &&
+
-+ git -C unmerged sparse-checkout disable &&
++ git -C unmerged sparse-checkout disable 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged reset --hard &&
17: fa997a1c72f ! 17: 844306c3e86 unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
@@ -37,6 +37,7 @@
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>
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
18: 5730f7d250a ! 18: 93dbdd78acf sparse-checkout: provide a new reapply subcommand
@@ -7,6 +7,7 @@
due to dirty changes, users may want a command to simply 'reapply' the
sparsity rules. Provide one.
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
@@ -64,3 +65,55 @@
if (!strcmp(argv[0], "disable"))
return sparse_checkout_disable(argc, argv);
}
+
+ diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
+ --- a/t/t1091-sparse-checkout-builtin.sh
+ +++ b/t/t1091-sparse-checkout-builtin.sh
+@@
+ git -C unmerged sparse-checkout disable
+ '
+
++test_expect_success 'sparse-checkout reapply' '
++ git clone repo tweak &&
++
++ echo dirty >tweak/deep/deeper2/a &&
++
++ cat >input <<-EOF &&
++ 0 0000000000000000000000000000000000000000 folder1/a
++ 100644 $(git -C tweak rev-parse HEAD:folder1/a) 1 folder1/a
++ EOF
++ git -C tweak update-index --index-info <input &&
++
++ git -C tweak sparse-checkout init --cone 2>err &&
++ test_i18ngrep "warning.*The following paths are not up to date" err &&
++ test_i18ngrep "warning.*The following paths are unmerged" err &&
++
++ git -C tweak sparse-checkout set folder2 deep/deeper1 2>err &&
++ test_i18ngrep "warning.*The following paths are not up to date" err &&
++ test_i18ngrep "warning.*The following paths are unmerged" err &&
++
++ git -C tweak sparse-checkout reapply 2>err &&
++ test_i18ngrep "warning.*The following paths are not up to date" err &&
++ test_path_is_file tweak/deep/deeper2/a &&
++ test_i18ngrep "warning.*The following paths are unmerged" err &&
++ test_path_is_file tweak/folder1/a &&
++
++ git -C tweak checkout HEAD deep/deeper2/a &&
++ git -C tweak sparse-checkout reapply 2>err &&
++ test_i18ngrep ! "warning.*The following paths are not up to date" err &&
++ test_path_is_missing tweak/deep/deeper2/a &&
++ test_i18ngrep "warning.*The following paths are unmerged" err &&
++ test_path_is_file tweak/folder1/a &&
++
++ git -C tweak add folder1/a &&
++ git -C tweak sparse-checkout reapply 2>err &&
++ test_must_be_empty err &&
++ test_path_is_missing tweak/deep/deeper2/a &&
++ test_path_is_missing tweak/folder1/a &&
++
++ git -C tweak sparse-checkout disable
++'
++
+ test_expect_success 'cone mode: set with core.ignoreCase=true' '
+ rm repo/.git/info/sparse-checkout &&
+ git -C repo sparse-checkout init --cone &&
--
gitgitgadget
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 01/18] unpack-trees: fix minor typo in comment
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
@ 2020-03-27 0:48 ` Elijah Newren via GitGitGadget
2020-03-27 0:48 ` [PATCH v3 02/18] unpack-trees: remove unused error type Elijah Newren via GitGitGadget
` (17 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 1ecdab33040..0d0eec0221e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1618,7 +1618,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
/*
* Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1
- * If the will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE
+ * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE
* so apply_sparse_checkout() won't attempt to remove it from worktree
*/
mark_new_skip_worktree(o->pl, &o->result,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 02/18] unpack-trees: remove unused error type
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 ` Elijah Newren via GitGitGadget
2020-03-27 0:48 ` [PATCH v3 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
` (16 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
commit 08402b0409 ("merge-recursive: distinguish "removed" and
"overwritten" messages", 2010-08-11) split
ERROR_WOULD_LOSE_UNTRACKED
into both
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN
ERROR_WOULD_LOSE_UNTRACKED_REMOVED
and also split
ERROR_WOULD_LOSE_ORPHANED
into both
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN
ERROR_WOULD_LOSE_ORPHANED_REMOVED
However, despite the split only three of these four types were used.
ERROR_WOULD_LOSE_ORPHANED_REMOVED was not put into use when it was
introduced and nothing else has used it in the intervening decade
either. Remove it.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 4 ----
unpack-trees.h | 1 -
2 files changed, 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 0d0eec0221e..f72a7a21f9c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -49,8 +49,6 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
- /* ERROR_WOULD_LOSE_ORPHANED_REMOVED */
- "Working tree file '%s' would be removed by sparse checkout update.",
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
@@ -172,8 +170,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
- msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
- _("The following working tree files would be removed by sparse checkout update:\n%s");
msgs[ERROR_WOULD_LOSE_SUBMODULE] =
_("Cannot update submodule:\n%s");
diff --git a/unpack-trees.h b/unpack-trees.h
index ae1557fb804..6d7c7b6c2e0 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -24,7 +24,6 @@ enum unpack_trees_error_types {
ERROR_BIND_OVERLAP,
ERROR_SPARSE_NOT_UPTODATE_FILE,
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- ERROR_WOULD_LOSE_ORPHANED_REMOVED,
ERROR_WOULD_LOSE_SUBMODULE,
NB_UNPACK_TREES_ERROR_TYPES
};
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 03/18] unpack-trees: simplify verify_absent_sparse()
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 ` Elijah Newren via GitGitGadget
2020-03-27 0:48 ` [PATCH v3 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
` (15 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
verify_absent_sparse() was introduced in commit 08402b0409
("merge-recursive: distinguish "removed" and "overwritten" messages",
2010-08-11), and has always had exactly one caller which always passes
error_type == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN. This function
then checks whether error_type is this value, and if so, sets it instead
to ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN. It has been nearly a decade
and no other caller has been created, and no other value has ever been
passed, so just pass the expected value to begin with.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f72a7a21f9c..3af2e126abf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -506,7 +506,7 @@ static int apply_sparse_checkout(struct index_state *istate,
ce->ce_flags &= ~CE_UPDATE;
}
if (was_skip_worktree && !ce_skip_worktree(ce)) {
- if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
+ if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o))
return -1;
ce->ce_flags |= CE_UPDATE;
}
@@ -2026,11 +2026,7 @@ static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types error_type,
struct unpack_trees_options *o)
{
- enum unpack_trees_error_types orphaned_error = error_type;
- if (orphaned_error == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN)
- orphaned_error = ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN;
-
- return verify_absent_1(ce, orphaned_error, o);
+ return verify_absent_1(ce, error_type, o);
}
static int merged_entry(const struct cache_entry *ce,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 04/18] unpack-trees: simplify pattern_list freeing
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2020-03-27 0:48 ` [PATCH v3 03/18] unpack-trees: simplify verify_absent_sparse() Elijah Newren via GitGitGadget
@ 2020-03-27 0:48 ` 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
` (14 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
commit e091228e17 ("sparse-checkout: update working directory
in-process", 2019-11-21) allowed passing a pre-defined set of patterns
to unpack_trees(). However, if o->pl was NULL, it would still read the
existing patterns and use those. If those patterns were read into a
data structure that was allocated, naturally they needed to be free'd.
However, despite the same function being responsible for knowing about
both the allocation and the free'ing, the logic for tracking whether to
free the pattern_list was hoisted to an outer function with an
additional flag in unpack_trees_options. Put the logic back in the
relevant function and discard the now unnecessary flag.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 1 -
unpack-trees.c | 6 ++++--
unpack-trees.h | 3 +--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 740da4b6d54..d102a9697fd 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl)
o.dst_index = r->index;
o.skip_sparse_checkout = 0;
o.pl = pl;
- o.keep_pattern_list = !!pl;
resolve_undo_clear_index(r->index);
setup_work_tree();
diff --git a/unpack-trees.c b/unpack-trees.c
index 3af2e126abf..d2863fa0310 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct pattern_list pl;
+ int free_pattern_list = 0;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
@@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
else
o->pl = &pl;
free(sparse);
+ free_pattern_list = 1;
}
memset(&o->result, 0, sizeof(o->result));
@@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
o->src_index = NULL;
done:
- trace_performance_leave("unpack_trees");
- if (!o->keep_pattern_list)
+ if (free_pattern_list)
clear_pattern_list(&pl);
+ trace_performance_leave("unpack_trees");
return ret;
return_failed:
diff --git a/unpack-trees.h b/unpack-trees.h
index 6d7c7b6c2e0..d3516267f36 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -58,8 +58,7 @@ struct unpack_trees_options {
quiet,
exiting_early,
show_all_errors,
- dry_run,
- keep_pattern_list;
+ dry_run;
const char *prefix;
int cache_bottom;
struct dir_struct *dir;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 05/18] t1091: make some tests a little more defensive against failures
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (3 preceding siblings ...)
2020-03-27 0:48 ` [PATCH v3 04/18] unpack-trees: simplify pattern_list freeing Elijah Newren via GitGitGadget
@ 2020-03-27 0:48 ` 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
` (13 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 44a91205d60..8607a8e6d1a 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -278,6 +278,7 @@ test_expect_success 'cone mode: add parent path' '
'
test_expect_success 'revert to old sparse-checkout on bad update' '
+ test_when_finished git -C repo sparse-checkout disable &&
test_when_finished git -C repo reset --hard &&
git -C repo sparse-checkout set deep &&
echo update >repo/deep/deeper2/a &&
@@ -328,6 +329,7 @@ test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status'
'
test_expect_success 'cone mode: set with core.ignoreCase=true' '
+ rm repo/.git/info/sparse-checkout &&
git -C repo sparse-checkout init --cone &&
git -C repo -c core.ignoreCase=true sparse-checkout set folder1 &&
cat >expect <<-\EOF &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 06/18] unpack-trees: allow check_updates() to work on a different index
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (4 preceding siblings ...)
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 ` 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
` (12 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
check_updates() previously assumed it was working on o->result. We want
to use this function in combination with a different index_state, so
take the intended index_state as a parameter.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index d2863fa0310..dde50047a82 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -353,12 +353,12 @@ static void report_collided_checkout(struct index_state *index)
string_list_clear(&list, 0);
}
-static int check_updates(struct unpack_trees_options *o)
+static int check_updates(struct unpack_trees_options *o,
+ struct index_state *index)
{
unsigned cnt = 0;
int errs = 0;
struct progress *progress;
- struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
int i;
@@ -1665,7 +1665,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
}
- ret = check_updates(o) ? (-2) : 0;
+ ret = check_updates(o, &o->result) ? (-2) : 0;
if (o->dst_index) {
move_index_extensions(&o->result, o->src_index);
if (!ret) {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 07/18] unpack-trees: do not mark a dirty path with SKIP_WORKTREE
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (5 preceding siblings ...)
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 ` 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
` (11 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
If a path is dirty, removing from the working tree risks losing data.
As such, we want to make sure any such path is not marked with
SKIP_WORKTREE. While the current callers of this code detect this case
and re-populate with a previous set of sparsity patterns, we want to
allow some paths to be marked with SKIP_WORKTREE while others are left
unmarked without it being considered an error. The reason this
shouldn't be considered an error is that SKIP_WORKTREE has always been
an advisory-only setting; merge and rebase for example were free to
materialize paths and clear the SKIP_WORKTREE bit in order to accomplish
their work even though they kept the SKIP_WORKTREE bit set for other
paths. Leaving dirty working files in the working tree is thus a
natural extension of what we have already been doing.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index dde50047a82..e8e794880ab 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -500,8 +500,11 @@ static int apply_sparse_checkout(struct index_state *istate,
* also stat info may have lost after merged_entry() so calling
* verify_uptodate() again may fail
*/
- if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
+ if (!(ce->ce_flags & CE_UPDATE) &&
+ verify_uptodate_sparse(ce, o)) {
+ ce->ce_flags &= ~CE_SKIP_WORKTREE;
return -1;
+ }
ce->ce_flags |= CE_WT_REMOVE;
ce->ce_flags &= ~CE_UPDATE;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 08/18] unpack-trees: pull sparse-checkout pattern reading into a new function
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (6 preceding siblings ...)
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 ` 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
` (10 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Create a populate_from_existing_patterns() function for reading the
path_patterns from $GIT_DIR/info/sparse-checkout so that we can re-use
it elsewhere.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index e8e794880ab..4733e7eaf89 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1492,6 +1492,20 @@ static void mark_new_skip_worktree(struct pattern_list *pl,
clear_ce_flags(istate, select_flag, skip_wt_flag, pl, show_progress);
}
+static void populate_from_existing_patterns(struct unpack_trees_options *o,
+ struct pattern_list *pl)
+{
+ char *sparse = git_pathdup("info/sparse-checkout");
+
+ pl->use_cone_patterns = core_sparse_checkout_cone;
+ if (add_patterns_from_file_to_list(sparse, "", 0, pl, NULL) < 0)
+ o->skip_sparse_checkout = 1;
+ else
+ o->pl = pl;
+ free(sparse);
+}
+
+
static int verify_absent(const struct cache_entry *,
enum unpack_trees_error_types,
struct unpack_trees_options *);
@@ -1512,18 +1526,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
trace_performance_enter();
- memset(&pl, 0, sizeof(pl));
if (!core_apply_sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout && !o->pl) {
- char *sparse = git_pathdup("info/sparse-checkout");
- pl.use_cone_patterns = core_sparse_checkout_cone;
- if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0)
- o->skip_sparse_checkout = 1;
- else
- o->pl = &pl;
- free(sparse);
+ memset(&pl, 0, sizeof(pl));
free_pattern_list = 1;
+ populate_from_existing_patterns(o, &pl);
}
memset(&o->result, 0, sizeof(o->result));
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 09/18] unpack-trees: add a new update_sparsity() function
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (7 preceding siblings ...)
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 ` Elijah Newren via GitGitGadget
2020-03-27 0:48 ` [PATCH v3 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
` (9 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
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 if
the index or working directory were not clean. First, let's consider
the case:
Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files)
If the working tree was clean this was fine, but if there were files or
directories or symlinks or whatever already present at the given path
then the operation would abort with an error. Let's label this case
for later discussion:
A) There is an untracked path in the way
Now let's consider the opposite case:
Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files)
If the index and working tree was clean this was fine, but if there were
any unclean paths we would run into problems. There are three different
cases to consider:
B) The path is unmerged
C) The path has unstaged changes
D) The path has staged changes (differs from HEAD)
If any path fell into case B or C, then the whole operation would be
aborted with an error. With sparse-checkout, the whole operation would
be aborted for case D as well, but for its predecessor of using `git
read-tree -mu HEAD` directly, any paths that fell into case D would be
removed from the working copy and the index entry for that path would be
reset to match HEAD -- which looks and feels like data loss to users
(only a few are even aware to ask whether it can be recovered, and even
then it requires walking through loose objects trying to match up the
right ones).
Refusing to remove files that have unsaved user changes is good, but
refusing to work on any other paths is very problematic for users. If
the user is in the middle of a rebase or has made modifications to files
that bring in more dependencies, then for their build to work they need
to update the sparse paths. This logic has been preventing them from
doing so. Sometimes in response, the user will stage the files and
re-try, to no avail with sparse-checkout or to the horror of losing
their changes if they are using its predecessor of `git read-tree -mu
HEAD`.
Add a new update_sparsity() function which will not error out in any of
these cases but behaves as follows for the special cases:
A) Leave the file in the working copy alone, clear the SKIP_WORKTREE
bit, and print a warning (thus leaving the path in a state where
status will report the file as modified, which seems logical).
B) Do NOT mark this path as SKIP_WORKTREE, and leave it as unmerged.
C) Do NOT mark this path as SKIP_WORKTREE and print a warning about
the dirty path.
D) 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 A (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
& C seem like optimal behavior to me. B may be as well, though I wonder
if printing a warning would be an improvement. Some might be slightly
surprised by D at first, but given that it does the right thing with
`git commit` and even `git commit -a` (`git add` ignores entries that
are marked SKIP_WORKTREE and thus doesn't delete them, and `commit -a`
is similar), it seems logical to me.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
unpack-trees.h | 9 ++++++
2 files changed, 86 insertions(+)
diff --git a/unpack-trees.c b/unpack-trees.c
index 4733e7eaf89..a5bc0a3a16d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1714,6 +1714,83 @@ 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.
+ *
+ * CE_NEW_SKIP_WORKTREE is used internally.
+ */
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
+{
+ enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
+ struct pattern_list pl;
+ int i, empty_worktree;
+ unsigned old_show_all_errors;
+ int free_pattern_list = 0;
+
+ 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 we weren't given patterns, use the recorded ones */
+ if (!o->pl) {
+ memset(&pl, 0, sizeof(pl));
+ free_pattern_list = 1;
+ populate_from_existing_patterns(o, &pl);
+ if (o->skip_sparse_checkout)
+ goto skip_sparse_checkout;
+ }
+
+ /* 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 = UPDATE_SPARSITY_SUCCESS;
+ 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 = UPDATE_SPARSITY_WARNINGS;
+
+ 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) {
+ unpack_failed(o, "Sparse checkout leaves no entry on working directory");
+ ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
+ goto done;
+ }
+
+skip_sparse_checkout:
+ if (check_updates(o, o->src_index))
+ ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
+
+done:
+ display_error_msgs(o);
+ o->show_all_errors = old_show_all_errors;
+ if (free_pattern_list)
+ clear_pattern_list(&pl);
+ 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 d3516267f36..5cf41ef5b53 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -88,6 +88,15 @@ struct unpack_trees_options {
int unpack_trees(unsigned n, struct tree_desc *t,
struct unpack_trees_options *options);
+enum update_sparsity_result {
+ UPDATE_SPARSITY_SUCCESS = 0,
+ UPDATE_SPARSITY_WARNINGS = 1,
+ UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
+ UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
+};
+
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *options);
+
int verify_uptodate(const struct cache_entry *ce,
struct unpack_trees_options *o);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 10/18] sparse-checkout: use new update_sparsity() function
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (8 preceding siblings ...)
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 ` 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
` (8 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout
codepaths for setting the SKIP_WORKTREE bits and instead use the new
update_sparsity() function.
Note that when an issue is hit, the error message splits 'error' and
'Cannot update sparse checkout' on separate lines. For now, we use two
greps to find both pieces of the error message but subsequent commits
will clean up the messages reported to the user.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 40 ++++++++----------------------
t/t1091-sparse-checkout-builtin.sh | 39 +++++++++++++++++++++--------
2 files changed, 39 insertions(+), 40 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index d102a9697fd..a55c60d7594 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -94,49 +94,35 @@ static int sparse_checkout_list(int argc, const char **argv)
static int update_working_directory(struct pattern_list *pl)
{
- int result = 0;
+ enum update_sparsity_result result;
struct unpack_trees_options o;
struct lock_file lock_file = LOCK_INIT;
- struct object_id oid;
- struct tree *tree;
- struct tree_desc t;
struct repository *r = the_repository;
- if (repo_read_index_unmerged(r))
- die(_("you need to resolve your current index first"));
-
- if (get_oid("HEAD", &oid))
- return 0;
-
- tree = parse_tree_indirect(&oid);
- parse_tree(tree);
- init_tree_desc(&t, tree->buffer, tree->size);
-
memset(&o, 0, sizeof(o));
o.verbose_update = isatty(2);
- o.merge = 1;
o.update = 1;
- o.fn = oneway_merge;
o.head_idx = -1;
o.src_index = r->index;
o.dst_index = r->index;
o.skip_sparse_checkout = 0;
o.pl = pl;
- resolve_undo_clear_index(r->index);
setup_work_tree();
- cache_tree_free(&r->index->cache_tree);
-
repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
- core_apply_sparse_checkout = 1;
- result = unpack_trees(1, &t, &o);
+ result = update_sparsity(&o);
- if (!result) {
- prime_cache_tree(r, r->index, tree);
+ if (result == UPDATE_SPARSITY_WARNINGS)
+ /*
+ * We don't do any special handling of warnings from untracked
+ * files in the way or dirty entries that can't be removed.
+ */
+ result = UPDATE_SPARSITY_SUCCESS;
+ if (result == UPDATE_SPARSITY_SUCCESS)
write_locked_index(r->index, &lock_file, COMMIT_LOCK);
- } else
+ else
rollback_lock_file(&lock_file);
return result;
@@ -303,8 +289,6 @@ static int sparse_checkout_init(int argc, const char **argv)
};
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("initialize sparse-checkout"), NULL, 1, 0);
argc = parse_options(argc, argv, NULL,
builtin_sparse_checkout_init_options,
@@ -559,8 +543,6 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
};
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("set sparse-checkout patterns"), NULL, 1, 0);
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
@@ -576,8 +558,6 @@ static int sparse_checkout_disable(int argc, const char **argv)
struct strbuf match_all = STRBUF_INIT;
repo_read_index(the_repository);
- require_clean_work_tree(the_repository,
- N_("disable sparse-checkout"), NULL, 1, 0);
memset(&pl, 0, sizeof(pl));
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 8607a8e6d1a..a991e0a80d5 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
check_files repo a deep folder1
'
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
test_when_finished git -C repo sparse-checkout disable &&
test_when_finished git -C repo reset --hard &&
git -C repo sparse-checkout set deep &&
+
echo update >repo/deep/deeper2/a &&
cp repo/.git/info/sparse-checkout expect &&
- test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
- test_i18ngrep "cannot set sparse-checkout patterns" err &&
- test_cmp repo/.git/info/sparse-checkout expect &&
- check_files repo/deep a deeper1 deeper2
+ test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
+
+ git -C repo sparse-checkout set deep/deeper1 2>err &&
+
+ test_i18ngrep "Cannot update sparse checkout" err &&
+ test_cmp expect repo/.git/info/sparse-checkout &&
+ check_files repo/deep a deeper1 deeper2 &&
+ check_files repo/deep/deeper1 a deepest &&
+ check_files repo/deep/deeper1/deepest a &&
+ check_files repo/deep/deeper2 a
'
test_expect_success 'revert to old sparse-checkout on empty update' '
@@ -316,16 +323,28 @@ test_expect_success '.gitignore should not warn about cone mode' '
test_i18ngrep ! "disabling cone patterns" err
'
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
git clone repo dirty &&
echo dirty >dirty/folder1/a &&
- test_must_fail git -C dirty sparse-checkout init &&
- test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- test_must_fail git -C dirty sparse-checkout disable &&
+
+ git -C dirty sparse-checkout init 2>err &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+
+ git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
+ test_i18ngrep "error" err &&
+ test_i18ngrep "Cannot update sparse checkout" err &&
+ test_path_is_file dirty/folder1/a &&
+
+ git -C dirty sparse-checkout disable 2>err &&
+ test_must_be_empty err &&
+
git -C dirty reset --hard &&
git -C dirty sparse-checkout init &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
- git -C dirty sparse-checkout disable
+ test_path_is_missing dirty/folder1/a &&
+ git -C dirty sparse-checkout disable &&
+ test_path_is_file dirty/folder1/a
'
test_expect_success 'cone mode: set with core.ignoreCase=true' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 11/18] sparse-checkout: use improved unpack_trees porcelain messages
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (9 preceding siblings ...)
2020-03-27 0:48 ` [PATCH v3 10/18] sparse-checkout: use " Elijah Newren via GitGitGadget
@ 2020-03-27 0:48 ` 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
` (7 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
setup_unpack_trees_porcelain() provides much improved error/warning
messages; instead of a message that assumes that there is only one path
with a given problem despite being used by code that intentionally is
grouping and showing errors together, it uses a message designed to be
used with groups of paths. For example, this transforms
error: Entry ' folder1/a
folder2/a
' not uptodate. Cannot update sparse checkout.
into
error: Cannot update sparse checkout: the following entries are not up to date:
folder1/a
folder2/a
In the past the suboptimal messages were never actually triggered
because we would error out if the working directory wasn't clean before
we even called unpack_trees(). The previous commit changed that,
though, so let's use the better error messages.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/sparse-checkout.c | 2 ++
t/t1091-sparse-checkout-builtin.sh | 6 ++----
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a55c60d7594..aa81199f85d 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -112,7 +112,9 @@ static int update_working_directory(struct pattern_list *pl)
repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
+ setup_unpack_trees_porcelain(&o, "sparse-checkout");
result = update_sparsity(&o);
+ clear_unpack_trees_porcelain(&o);
if (result == UPDATE_SPARSITY_WARNINGS)
/*
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index a991e0a80d5..9bc65d32f07 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -328,12 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "error" err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "error.*Cannot update sparse checkout" err &&
test_path_is_file dirty/folder1/a &&
git -C dirty sparse-checkout disable 2>err &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 12/18] unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (10 preceding siblings ...)
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 ` 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
` (6 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
A minor change, but we want to convert the sparse messages to warnings
and this allows us to group warnings and errors.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 12 ++++++------
unpack-trees.h | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index a5bc0a3a16d..eeac309e30e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -43,15 +43,14 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_BIND_OVERLAP */
"Entry '%s' overlaps with '%s'. Cannot bind.",
+ /* ERROR_WOULD_LOSE_SUBMODULE */
+ "Submodule '%s' cannot checkout new HEAD.",
+
/* ERROR_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
/* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
-
-
- /* ERROR_WOULD_LOSE_SUBMODULE */
- "Submodule '%s' cannot checkout new HEAD.",
};
#define ERRORMSG(o,type) \
@@ -166,12 +165,13 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
*/
msgs[ERROR_BIND_OVERLAP] = _("Entry '%s' overlaps with '%s'. Cannot bind.");
+ msgs[ERROR_WOULD_LOSE_SUBMODULE] =
+ _("Cannot update submodule:\n%s");
+
msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
- msgs[ERROR_WOULD_LOSE_SUBMODULE] =
- _("Cannot update submodule:\n%s");
opts->show_all_errors = 1;
/* rejected paths may not have a static buffer */
diff --git a/unpack-trees.h b/unpack-trees.h
index 5cf41ef5b53..3e996a6c0a9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -22,9 +22,9 @@ enum unpack_trees_error_types {
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,
ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
ERROR_BIND_OVERLAP,
+ ERROR_WOULD_LOSE_SUBMODULE,
ERROR_SPARSE_NOT_UPTODATE_FILE,
ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- ERROR_WOULD_LOSE_SUBMODULE,
NB_UNPACK_TREES_ERROR_TYPES
};
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 13/18] unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (11 preceding siblings ...)
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 ` 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
` (5 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
We want to treat issues with setting the SKIP_WORKTREE bit as a warning
rather than an error; rename the enum values to reflect this intent as
a simple step towards that goal.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
unpack-trees.c | 12 ++++++------
unpack-trees.h | 8 +++++---
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index eeac309e30e..2a2306c5c28 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -46,10 +46,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
- /* ERROR_SPARSE_NOT_UPTODATE_FILE */
+ /* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
- /* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */
+ /* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
"Working tree file '%s' would be overwritten by sparse checkout update.",
};
@@ -168,9 +168,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
msgs[ERROR_WOULD_LOSE_SUBMODULE] =
_("Cannot update submodule:\n%s");
- msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
+ msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
_("Cannot update sparse checkout: the following entries are not up to date:\n%s");
- msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
+ msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
_("The following working tree files would be overwritten by sparse checkout update:\n%s");
opts->show_all_errors = 1;
@@ -509,7 +509,7 @@ static int apply_sparse_checkout(struct index_state *istate,
ce->ce_flags &= ~CE_UPDATE;
}
if (was_skip_worktree && !ce_skip_worktree(ce)) {
- if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o))
+ if (verify_absent_sparse(ce, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, o))
return -1;
ce->ce_flags |= CE_UPDATE;
}
@@ -1875,7 +1875,7 @@ int verify_uptodate(const struct cache_entry *ce,
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
- return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);
+ return verify_uptodate_1(ce, o, WARNING_SPARSE_NOT_UPTODATE_FILE);
}
/*
diff --git a/unpack-trees.h b/unpack-trees.h
index 3e996a6c0a9..aac1ad4b014 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -23,9 +23,11 @@ enum unpack_trees_error_types {
ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
ERROR_BIND_OVERLAP,
ERROR_WOULD_LOSE_SUBMODULE,
- ERROR_SPARSE_NOT_UPTODATE_FILE,
- ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
- NB_UNPACK_TREES_ERROR_TYPES
+
+ WARNING_SPARSE_NOT_UPTODATE_FILE,
+ WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
+
+ NB_UNPACK_TREES_ERROR_TYPES,
};
/*
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 14/18] unpack-trees: split display_error_msgs() into two
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (12 preceding siblings ...)
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 ` 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
` (4 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
display_error_msgs() is never called to show messages of both ERROR_*
and WARNING_* types at the same time; it is instead called multiple
times, separately for each type. Since we want to display these types
differently, make two slightly different versions of this function.
A subsequent commit will further modify unpack_trees() and how it calls
the new display_warning_msgs().
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 4 +--
unpack-trees.c | 50 +++++++++++++++++++++++++-----
unpack-trees.h | 8 +++--
3 files changed, 49 insertions(+), 13 deletions(-)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 9bc65d32f07..ed5e9059969 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -328,10 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "error.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*Cannot update sparse checkout" err &&
test_path_is_file dirty/folder1/a &&
git -C dirty sparse-checkout disable 2>err &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 2a2306c5c28..f9a5626a670 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -24,7 +24,7 @@
* situation better. See how "git checkout" and "git merge" replaces
* them using setup_unpack_trees_porcelain(), for example.
*/
-static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
+static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
/* ERROR_WOULD_OVERWRITE */
"Entry '%s' would be overwritten by merge. Cannot merge.",
@@ -46,6 +46,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_WOULD_LOSE_SUBMODULE */
"Submodule '%s' cannot checkout new HEAD.",
+ /* NB_UNPACK_TREES_ERROR_TYPES; just a meta value */
+ "",
+
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Entry '%s' not uptodate. Cannot update sparse checkout.",
@@ -222,7 +225,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
/*
* Otherwise, insert in a list for future display by
- * display_error_msgs()
+ * display_(error|warning)_msgs()
*/
string_list_append(&o->unpack_rejects[e], path);
return -1;
@@ -233,13 +236,16 @@ static int add_rejected_path(struct unpack_trees_options *o,
*/
static void display_error_msgs(struct unpack_trees_options *o)
{
- int e, i;
- int something_displayed = 0;
+ int e;
+ unsigned error_displayed = 0;
for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) {
struct string_list *rejects = &o->unpack_rejects[e];
+
if (rejects->nr > 0) {
+ int i;
struct strbuf path = STRBUF_INIT;
- something_displayed = 1;
+
+ error_displayed = 1;
for (i = 0; i < rejects->nr; i++)
strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
error(ERRORMSG(o, e), super_prefixed(path.buf));
@@ -247,10 +253,36 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
string_list_clear(rejects, 0);
}
- if (something_displayed)
+ if (error_displayed)
fprintf(stderr, _("Aborting\n"));
}
+/*
+ * display all the warning messages stored in a nice way
+ */
+static void display_warning_msgs(struct unpack_trees_options *o)
+{
+ int e;
+ unsigned warning_displayed = 0;
+ for (e = NB_UNPACK_TREES_ERROR_TYPES + 1;
+ e < NB_UNPACK_TREES_WARNING_TYPES; e++) {
+ struct string_list *rejects = &o->unpack_rejects[e];
+
+ if (rejects->nr > 0) {
+ int i;
+ struct strbuf path = STRBUF_INIT;
+
+ warning_displayed = 1;
+ for (i = 0; i < rejects->nr; i++)
+ strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
+ warning(ERRORMSG(o, e), super_prefixed(path.buf));
+ strbuf_release(&path);
+ }
+ string_list_clear(rejects, 0);
+ }
+ if (warning_displayed)
+ fprintf(stderr, _("After fixing the above paths, you may want to run `git sparse-checkout reapply`.\n"));
+}
static int check_submodule_move_head(const struct cache_entry *ce,
const char *old_id,
const char *new_id,
@@ -1705,8 +1737,10 @@ 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)
@@ -1783,7 +1817,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
done:
- display_error_msgs(o);
+ display_warning_msgs(o);
o->show_all_errors = old_show_all_errors;
if (free_pattern_list)
clear_pattern_list(&pl);
diff --git a/unpack-trees.h b/unpack-trees.h
index aac1ad4b014..dae948205f9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -24,10 +24,12 @@ enum unpack_trees_error_types {
ERROR_BIND_OVERLAP,
ERROR_WOULD_LOSE_SUBMODULE,
+ NB_UNPACK_TREES_ERROR_TYPES,
+
WARNING_SPARSE_NOT_UPTODATE_FILE,
WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
- NB_UNPACK_TREES_ERROR_TYPES,
+ NB_UNPACK_TREES_WARNING_TYPES,
};
/*
@@ -66,13 +68,13 @@ struct unpack_trees_options {
struct dir_struct *dir;
struct pathspec *pathspec;
merge_fn_t fn;
- const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+ const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
struct argv_array msgs_to_free;
/*
* Store error messages in an array, each case
* corresponding to a error message type
*/
- struct string_list unpack_rejects[NB_UNPACK_TREES_ERROR_TYPES];
+ struct string_list unpack_rejects[NB_UNPACK_TREES_WARNING_TYPES];
int head_idx;
int merge_size;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 15/18] unpack-trees: make sparse path messages sound like warnings
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (13 preceding siblings ...)
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 ` 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
` (3 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
The messages for problems with sparse paths are phrased as errors that
cause the operation to abort, even though we are not making the
operation abort. Reword the messages to make sense in their new
context.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 6 +++---
unpack-trees.c | 8 ++++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index ed5e9059969..afbde89e605 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -288,7 +288,7 @@ test_expect_success 'not-up-to-date does not block rest of sparsification' '
git -C repo sparse-checkout set deep/deeper1 2>err &&
- test_i18ngrep "Cannot update sparse checkout" err &&
+ test_i18ngrep "The following paths are not up to date" err &&
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo/deep a deeper1 deeper2 &&
check_files repo/deep/deeper1 a deepest &&
@@ -328,10 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
echo dirty >dirty/folder1/a &&
git -C dirty sparse-checkout init 2>err &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
- test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
test_path_is_file dirty/folder1/a &&
git -C dirty sparse-checkout disable 2>err &&
diff --git a/unpack-trees.c b/unpack-trees.c
index f9a5626a670..484d30a53a7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -50,10 +50,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
"",
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
- "Entry '%s' not uptodate. Cannot update sparse checkout.",
+ "Path '%s' not uptodate; will not remove from working tree.",
/* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
- "Working tree file '%s' would be overwritten by sparse checkout update.",
+ "Path '%s' already present; will not overwrite with sparse update.",
};
#define ERRORMSG(o,type) \
@@ -172,9 +172,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
_("Cannot update submodule:\n%s");
msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
- _("Cannot update sparse checkout: the following entries are not up to date:\n%s");
+ _("The following paths are not up to date and were left despite sparse patterns:\n%s");
msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
- _("The following working tree files would be overwritten by sparse checkout update:\n%s");
+ _("The following paths were already present and thus not updated despite sparse patterns:\n%s");
opts->show_all_errors = 1;
/* rejected paths may not have a static buffer */
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 16/18] unpack-trees: provide warnings on sparse updates for unmerged paths too
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (14 preceding siblings ...)
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 ` 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
` (2 subsequent siblings)
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:48 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
When sparse-checkout runs to update the list of sparsity patterns, it
gives warnings if it can't remove paths from the working tree because
those files have dirty changes. Add a similar warning for unmerged
paths as well.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t1091-sparse-checkout-builtin.sh | 25 +++++++++++++++++++++++++
unpack-trees.c | 30 ++++++++++++++++++++++++++++++
unpack-trees.h | 1 +
3 files changed, 56 insertions(+)
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index afbde89e605..8e2976bc7b8 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -345,6 +345,31 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
test_path_is_file dirty/folder1/a
'
+test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged status' '
+ git clone repo unmerged &&
+
+ cat >input <<-EOF &&
+ 0 0000000000000000000000000000000000000000 folder1/a
+ 100644 $(git -C unmerged rev-parse HEAD:folder1/a) 1 folder1/a
+ EOF
+ git -C unmerged update-index --index-info <input &&
+
+ git -C unmerged sparse-checkout init 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+ test_path_is_file dirty/folder1/a &&
+
+ git -C unmerged sparse-checkout disable 2>err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C unmerged reset --hard &&
+ git -C unmerged sparse-checkout init &&
+ git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
+ git -C unmerged sparse-checkout disable
+'
+
test_expect_success 'cone mode: set with core.ignoreCase=true' '
rm repo/.git/info/sparse-checkout &&
git -C repo sparse-checkout init --cone &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 484d30a53a7..dec044339df 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
/* WARNING_SPARSE_NOT_UPTODATE_FILE */
"Path '%s' not uptodate; will not remove from working tree.",
+ /* WARNING_SPARSE_UNMERGED_FILE */
+ "Path '%s' unmerged; will not remove from working tree.",
+
/* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */
"Path '%s' already present; will not overwrite with sparse update.",
};
@@ -173,6 +176,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] =
_("The following paths are not up to date and were left despite sparse patterns:\n%s");
+ msgs[WARNING_SPARSE_UNMERGED_FILE] =
+ _("The following paths are unmerged and were left despite sparse patterns:\n%s");
msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] =
_("The following paths were already present and thus not updated despite sparse patterns:\n%s");
@@ -548,6 +553,23 @@ static int apply_sparse_checkout(struct index_state *istate,
return 0;
}
+static int warn_conflicted_path(struct index_state *istate,
+ int i,
+ struct unpack_trees_options *o)
+{
+ char *conflicting_path = istate->cache[i]->name;
+ int count = 0;
+
+ add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
+
+ /* Find out how many higher stage entries at same path */
+ while (++count < istate->cache_nr &&
+ !strcmp(conflicting_path,
+ istate->cache[i+count]->name))
+ /* do nothing */;
+ return count;
+}
+
static inline int call_unpack_fn(const struct cache_entry * const *src,
struct unpack_trees_options *o)
{
@@ -1793,6 +1815,14 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
for (i = 0; i < o->src_index->cache_nr; i++) {
struct cache_entry *ce = o->src_index->cache[i];
+
+ if (ce_stage(ce)) {
+ /* -1 because for loop will increment by 1 */
+ i += warn_conflicted_path(o->src_index, i, o) - 1;
+ ret = UPDATE_SPARSITY_WARNINGS;
+ continue;
+ }
+
if (apply_sparse_checkout(o->src_index, ce, o))
ret = UPDATE_SPARSITY_WARNINGS;
diff --git a/unpack-trees.h b/unpack-trees.h
index dae948205f9..0f7424aec61 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -27,6 +27,7 @@ enum unpack_trees_error_types {
NB_UNPACK_TREES_ERROR_TYPES,
WARNING_SPARSE_NOT_UPTODATE_FILE,
+ WARNING_SPARSE_UNMERGED_FILE,
WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN,
NB_UNPACK_TREES_WARNING_TYPES,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (15 preceding siblings ...)
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
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
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:49 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 18/18] sparse-checkout: provide a new reapply subcommand
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (16 preceding siblings ...)
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 ` Elijah Newren via GitGitGadget
2020-03-27 13:22 ` [PATCH v3 00/18] Sparse checkout improvements -- improved sparsity updating Derrick Stolee
18 siblings, 0 replies; 69+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-03-27 0:49 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
If commands like merge or rebase materialize files as part of their work,
or a previous sparse-checkout command failed to update individual files
due to dirty changes, users may want a command to simply 'reapply' the
sparsity rules. Provide one.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-sparse-checkout.txt | 10 +++++++
builtin/sparse-checkout.c | 10 ++++++-
t/t1091-sparse-checkout-builtin.sh | 41 +++++++++++++++++++++++++++
3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index c0342e53938..1a3ace60820 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -70,6 +70,16 @@ C-style quoted strings.
`core.sparseCheckoutCone` is enabled, the given patterns are interpreted
as directory names as in the 'set' subcommand.
+'reapply::
+ Reapply the sparsity pattern rules to paths in the working tree.
+ Commands like merge or rebase can materialize paths to do their
+ work (e.g. in order to show you a conflict), and other
+ sparse-checkout commands might fail to sparsify an individual file
+ (e.g. because it has unstaged changes or conflicts). In such
+ cases, it can make sense to run `git sparse-checkout reapply` later
+ after cleaning up affected paths (e.g. resolving conflicts, undoing
+ or committing changes, etc.).
+
'disable'::
Disable the `core.sparseCheckout` config setting, and restore the
working directory to include all files. Leaves the sparse-checkout
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index aa81199f85d..95d08824172 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -18,7 +18,7 @@
static const char *empty_base = "";
static char const * const builtin_sparse_checkout_usage[] = {
- N_("git sparse-checkout (init|list|set|add|disable) <options>"),
+ N_("git sparse-checkout (init|list|set|add|reapply|disable) <options>"),
NULL
};
@@ -554,6 +554,12 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
return modify_pattern_list(argc, argv, m);
}
+static int sparse_checkout_reapply(int argc, const char **argv)
+{
+ repo_read_index(the_repository);
+ return update_working_directory(NULL);
+}
+
static int sparse_checkout_disable(int argc, const char **argv)
{
struct pattern_list pl;
@@ -603,6 +609,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
return sparse_checkout_set(argc, argv, prefix, REPLACE);
if (!strcmp(argv[0], "add"))
return sparse_checkout_set(argc, argv, prefix, ADD);
+ if (!strcmp(argv[0], "reapply"))
+ return sparse_checkout_reapply(argc, argv);
if (!strcmp(argv[0], "disable"))
return sparse_checkout_disable(argc, argv);
}
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 8e2976bc7b8..dee99eeec30 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -370,6 +370,47 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat
git -C unmerged sparse-checkout disable
'
+test_expect_success 'sparse-checkout reapply' '
+ git clone repo tweak &&
+
+ echo dirty >tweak/deep/deeper2/a &&
+
+ cat >input <<-EOF &&
+ 0 0000000000000000000000000000000000000000 folder1/a
+ 100644 $(git -C tweak rev-parse HEAD:folder1/a) 1 folder1/a
+ EOF
+ git -C tweak update-index --index-info <input &&
+
+ git -C tweak sparse-checkout init --cone 2>err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C tweak sparse-checkout set folder2 deep/deeper1 2>err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+
+ git -C tweak sparse-checkout reapply 2>err &&
+ test_i18ngrep "warning.*The following paths are not up to date" err &&
+ test_path_is_file tweak/deep/deeper2/a &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+ test_path_is_file tweak/folder1/a &&
+
+ git -C tweak checkout HEAD deep/deeper2/a &&
+ git -C tweak sparse-checkout reapply 2>err &&
+ test_i18ngrep ! "warning.*The following paths are not up to date" err &&
+ test_path_is_missing tweak/deep/deeper2/a &&
+ test_i18ngrep "warning.*The following paths are unmerged" err &&
+ test_path_is_file tweak/folder1/a &&
+
+ git -C tweak add folder1/a &&
+ git -C tweak sparse-checkout reapply 2>err &&
+ test_must_be_empty err &&
+ test_path_is_missing tweak/deep/deeper2/a &&
+ test_path_is_missing tweak/folder1/a &&
+
+ git -C tweak sparse-checkout disable
+'
+
test_expect_success 'cone mode: set with core.ignoreCase=true' '
rm repo/.git/info/sparse-checkout &&
git -C repo sparse-checkout init --cone &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v3 00/18] Sparse checkout improvements -- improved sparsity updating
2020-03-27 0:48 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (17 preceding siblings ...)
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 ` Derrick Stolee
18 siblings, 0 replies; 69+ messages in thread
From: Derrick Stolee @ 2020-03-27 13:22 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Derrick Stolee, Elijah Newren
On 3/26/2020 8:48 PM, Elijah Newren via GitGitGadget wrote:
> This series provides a replacement for the sparsity updating in
> sparse-checkout that is based on the logic from git read-tree -mu HEAD. The
> most important bit is patch 9 and its lengthy commit message explaining the
> current state and rationale for most the series, though patches 16 and 17
> have additional related directions and rationale for the series. Those three
> patches are the most important to review.
>
> Changes since v2:
>
> * addressed Stolee's cleanups and added his Reviewed-by to the series
> (hopefully the next two changes don't invalidate that)
The other changes below look good to me. My Reviewed-by stands.
> * added a test for the new 'reapply' subcommand (noticed it was missing
> with Stolee's test coverage report)
> * fix a minor issue with two of the other tests I modified -- when I'm
> grepping through stderr for a command, I need to make sure to actually
> record stderr from that command (otherwise my grep is looking through the
> stderr of a previous command that was recorded). Also, since I messed
> this up, I added one or two more sanity checks while I was at it.
Thanks!
-Stolee
^ permalink raw reply [flat|nested] 69+ messages in thread