On Mon, Sep 20, 2021 at 10:45 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
This series is based on ds/mergies-with-sparse-index.
As requested, this series looks to update the behavior of git add, git rm,
and git mv when they attempt to modify paths outside of the sparse-checkout
cone. In particular, this care is expanded to not just cache entries with
the SKIP_WORKTREE bit, but also paths that do not match the sparse-checkout
definition.
This means that commands that worked before this series can now fail. In
particular, if 'git merge' results in a conflict outside of the
sparse-checkout cone, then 'git add ' will now fail.
In order to allow users to circumvent these protections, a new '--sparse'
option is added that ignores the sparse-checkout patterns and the
SKIP_WORKTREE bit. The message for advice.updateSparsePath is adjusted to
assist with discovery of this option.
There is a subtle issue with git mv in that it does not check the index
until it discovers a directory and then uses the index to find the contained
entries. This means that in non-cone-mode patterns, a pattern such as
"sub/dir" will not match the path "sub" and this can cause an issue.
In order to allow for checking arbitrary paths against the sparse-checkout
patterns, some changes to the underlying pattern matching code is required.
It turns out that there are some bugs in the methods as advertised, but
these bugs were never discovered because of the way methods like
unpack_trees() will check a directory for a pattern match before checking
its contained paths. Our new "check patterns on-demand" approach pokes holes
in that approach, specifically with patterns that match entire directories.
Updates in v3
=============
* Fixed an incorrectly-squashed commit. Spread out some changes in a better
way. For example, I don't add --sparse to tests before introducing the
option.
* Use a NULL struct strbuf pointer to indicate an uninitialized value
instead of relying on an internal member.
* Use grep over test_i18ngrep.
* Fixed line wrapping for error messages.
* Use strbuf_setlen() over modifying the len member manually.
I see that you and Junio had some interesting comments on the first 3
patches, so I look forward to seeing how those play out, but you
addressed all my feedback from the previous rounds here.
From: René Scharfe <hidden> Date: 2021-09-24 07:44:41
Am 24.08.21 um 23:54 schrieb Derrick Stolee via GitGitGadget:
quoted hunk
From: Derrick Stolee <redacted>
When matching a path against a list of patterns, the ones that require a
directory match previously did not work when a filename is specified.
This was fine when all pattern-matching was done within methods such as
unpack_trees() that check a directory before recursing into the
contained files. However, other commands will start matching individual
files against pattern lists without that recursive approach.
We modify path_matches_dir_pattern() to take a strbuf 'path_parent' that
is used to store the parent directory of 'pathname' between multiple
pattern matching tests. This is loaded lazily, only on the first pattern
it finds that has the PATTERN_FLAG_MUSTBEDIR flag.
If we find that a path has a parent directory, we start by checking to
see if that parent directory matches the pattern. If so, then we do not
need to query the index for the type (which can be expensive). If we
find that the parent does not match, then we still must check the type
from the index for the given pathname.
Note that this does not affect cone mode pattern matching, but instead
the more general -- and slower -- full pattern set. Thus, this does not
affect the sparse index.
Signed-off-by: Derrick Stolee <redacted>
---
dir.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
@@ -1305,10 +1305,38 @@ int match_pathname(const char *pathname, int pathlen,staticintpath_matches_dir_pattern(constchar*pathname,intpathlen,+structstrbuf*path_parent,int*dtype,structpath_pattern*pattern,structindex_state*istate){+/*+*Use'alloc'asanindicatorthatthestringhasnotbeen+*initialized,incasetheparentistherootdirectory.+*/
This means the caller needs to take care to release the strbuf between
calls for files from different directories. Seems a bit fragile. The
current caller is only ever passing in the same pathname before throwing
away the strbuf, so it's doing the right thing.
The caller has pathname, pathlen and basename. If basename is
guaranteed to be a substring of pathname then the parent directory name
length could be calculated without requiring a string copy or scan.
IIUC if pathname and basename can be pointers to different objects then
just checking if basename is between pathname and pathname + pathlen
would already be undefined behavior.
Using pathname, pathlen and dirlen instead would be safer for such
calculations, as it enforces basename to be a substring. Seems like
this would require a lot of function signature changes, though, as the
call tree is quite high. :|
+
+ if (slash)
+ *slash = '\0';
This doesn't update path_parent->len...
+ else
+ strbuf_setlen(path_parent, 0);
+ }
+
+ /*
+ * If the parent directory matches the pattern, then we do not
+ * need to check for dtype.
+ */
+ if (path_parent->len &&
+ match_pathname(path_parent->buf, path_parent->len,
... so this checks if "<dirname>\0<basename>" matches. Intended?
"a & b && c" is equivalent to "(a & b) && c", but removing the
parentheses here serves no apparent purpose and distracts a bit from
the actual change, i.e. adding a parameter.
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:26
This series is based on ds/mergies-with-sparse-index.
As requested, this series looks to update the behavior of git add, git rm,
and git mv when they attempt to modify paths outside of the sparse-checkout
cone. In particular, this care is expanded to not just cache entries with
the SKIP_WORKTREE bit, but also paths that do not match the sparse-checkout
definition.
This means that commands that worked before this series can now fail. In
particular, if 'git merge' results in a conflict outside of the
sparse-checkout cone, then 'git add ' will now fail.
In order to allow users to circumvent these protections, a new '--sparse'
option is added that ignores the sparse-checkout patterns and the
SKIP_WORKTREE bit. The message for advice.updateSparsePath is adjusted to
assist with discovery of this option.
There is a subtle issue with git mv in that it does not check the index
until it discovers a directory and then uses the index to find the contained
entries. This means that in non-cone-mode patterns, a pattern such as
"sub/dir" will not match the path "sub" and this can cause an issue.
In order to allow for checking arbitrary paths against the sparse-checkout
patterns, some changes to the underlying pattern matching code is required.
It turns out that there are some bugs in the methods as advertised, but
these bugs were never discovered because of the way methods like
unpack_trees() will check a directory for a pattern match before checking
its contained paths. Our new "check patterns on-demand" approach pokes holes
in that approach, specifically with patterns that match entire directories.
Updates in v4
=============
* Instead of using 'git status' and 'grep' to detect staged changes, we use
'git diff --staged'. t1092 uses an additional --diff-filter because it
tests with merge conflicts, so it needs this extra flag.
* Patches 3 and 4 are merged into the new patch 3 to avoid temporarily
having a poorly named method.
Updates in v3
=============
* Fixed an incorrectly-squashed commit. Spread out some changes in a better
way. For example, I don't add --sparse to tests before introducing the
option.
* Use a NULL struct strbuf pointer to indicate an uninitialized value
instead of relying on an internal member.
* Use grep over test_i18ngrep.
* Fixed line wrapping for error messages.
* Use strbuf_setlen() over modifying the len member manually.
Updates in v2
=============
* I got no complaints about these restrictions, so this is now a full
series, not RFC.
* Thanks to Matheus, several holes are filled with extra testing and
bugfixes.
* New patches add --chmod and --renormalize improvements. These are added
after the --sparse option to make them be one change each.
Thanks, -Stolee
Derrick Stolee (13):
t3705: test that 'sparse_entry' is unstaged
t1092: behavior for adding sparse files
dir: select directories correctly
dir: fix pattern matching on dirs
add: fail when adding an untracked sparse file
add: skip tracked paths outside sparse-checkout cone
add: implement the --sparse option
add: update --chmod to skip sparse paths
add: update --renormalize to skip sparse paths
rm: add --sparse option
rm: skip sparse paths with missing SKIP_WORKTREE
mv: refuse to move sparse paths
advice: update message to suggest '--sparse'
Documentation/git-add.txt | 9 +-
Documentation/git-rm.txt | 6 +
advice.c | 11 +-
builtin/add.c | 32 +++-
builtin/mv.c | 52 +++++--
builtin/rm.c | 10 +-
dir.c | 56 ++++++-
pathspec.c | 5 +-
t/t1091-sparse-checkout-builtin.sh | 4 +-
t/t1092-sparse-checkout-compatibility.sh | 75 +++++++--
t/t3602-rm-sparse-checkout.sh | 40 ++++-
t/t3705-add-sparse-checkout.sh | 68 +++++++-
t/t7002-mv-sparse-checkout.sh | 189 +++++++++++++++++++++++
13 files changed, 505 insertions(+), 52 deletions(-)
create mode 100755 t/t7002-mv-sparse-checkout.sh
base-commit: 516680ba7704c473bb21628aa19cabbd787df4db
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1018%2Fderrickstolee%2Fsparse-index%2Fadd-rm-mv-behavior-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1018/derrickstolee/sparse-index/add-rm-mv-behavior-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1018
Range-diff vs v3:
1: ea940f10a7c ! 1: 642b05fc020 t3705: test that 'sparse_entry' is unstaged
@@ t/t3705-add-sparse-checkout.sh: setup_gitignore () {
}
+test_sparse_entry_unstaged () {
-+ git status --porcelain >actual &&
-+ ! grep "^[MDARCU][M ] sparse_entry\$" actual
++ git diff --staged -- sparse_entry >diff &&
++ test_must_be_empty diff
+}
+
test_expect_success 'setup' "
2: c7dedb41291 ! 2: 58389edc76c t1092: behavior for adding sparse files
@@ t/t1092-sparse-checkout-compatibility.sh: test_sparse_match () {
+ file=$1 &&
+ for repo in sparse-checkout sparse-index
+ do
-+ git -C $repo status --porcelain >$repo-out &&
-+ ! grep "^A $file\$" $repo-out &&
-+ ! grep "^M $file\$" $repo-out || return 1
++ # Skip "unmerged" paths
++ git -C $repo diff --staged --diff-filter=ACDMRTXB -- "$file" >diff &&
++ test_must_be_empty diff || return 1
+ done
+}
+
3: b1f6468f9cd < -: ----------- dir: extract directory-matching logic
4: 0252c7ee15c ! 3: 2ebaf8e68c2 dir: select directories correctly
@@ Commit message
contained files. However, other commands will start matching individual
files against pattern lists without that recursive approach.
- We modify path_matches_dir_pattern() to take a strbuf pointer
- 'path_parent' that is used to store the parent directory of 'pathname'
- between multiple pattern matching tests. This is loaded lazily, only on
- the first pattern it finds that has the PATTERN_FLAG_MUSTBEDIR flag.
+ The last_matching_pattern_from_list() logic performs some checks on the
+ filetype of a path within the index when the PATTERN_FLAG_MUSTBEDIR flag
+ is set. This works great when setting SKIP_WORKTREE bits within
+ unpack_trees(), but doesn't work well when passing an arbitrary path
+ such as a file within a matching directory.
+
+ We extract the logic around determining the file type, but attempt to
+ avoid checking the filesystem if the parent directory already matches
+ the sparse-checkout patterns. The new path_matches_dir_pattern() method
+ includes a 'path_parent' parameter that is used to store the parent
+ directory of 'pathname' between multiple pattern matching tests. This is
+ loaded lazily, only on the first pattern it finds that has the
+ PATTERN_FLAG_MUSTBEDIR flag.
If we find that a path has a parent directory, we start by checking to
see if that parent directory matches the pattern. If so, then we do not
@@ Commit message
## dir.c ##
@@ dir.c: int match_pathname(const char *pathname, int pathlen,
+ WM_PATHNAME) == 0;
+ }
- static int path_matches_dir_pattern(const char *pathname,
- int pathlen,
++static int path_matches_dir_pattern(const char *pathname,
++ int pathlen,
+ struct strbuf **path_parent,
- int *dtype,
- struct path_pattern *pattern,
- struct index_state *istate)
- {
++ int *dtype,
++ struct path_pattern *pattern,
++ struct index_state *istate)
++{
+ if (!*path_parent) {
+ char *slash;
+ CALLOC_ARRAY(*path_parent, 1);
@@ dir.c: int match_pathname(const char *pathname, int pathlen,
+ pattern->patternlen, pattern->flags))
+ return 1;
+
- *dtype = resolve_dtype(*dtype, istate, pathname, pathlen);
- if (*dtype != DT_DIR)
- return 0;
++ *dtype = resolve_dtype(*dtype, istate, pathname, pathlen);
++ if (*dtype != DT_DIR)
++ return 0;
++
++ return 1;
++}
++
+ /*
+ * Scan the given exclude list in reverse to see whether pathname
+ * should be ignored. The first match (i.e. the last on the list), if
@@ dir.c: static struct path_pattern *last_matching_pattern_from_list(const char *pathname
{
struct path_pattern *res = NULL; /* undecided */
@@ dir.c: static struct path_pattern *last_matching_pattern_from_list(const char *p
const char *exclude = pattern->pattern;
int prefix = pattern->nowildcardlen;
-- if ((pattern->flags & PATTERN_FLAG_MUSTBEDIR) &&
-- !path_matches_dir_pattern(pathname, pathlen,
+- if (pattern->flags & PATTERN_FLAG_MUSTBEDIR) {
+- *dtype = resolve_dtype(*dtype, istate, pathname, pathlen);
+- if (*dtype != DT_DIR)
+- continue;
+- }
+ if (pattern->flags & PATTERN_FLAG_MUSTBEDIR &&
+ !path_matches_dir_pattern(pathname, pathlen, &path_parent,
- dtype, pattern, istate))
- continue;
++ dtype, pattern, istate))
++ continue;
+ if (pattern->flags & PATTERN_FLAG_NODIR) {
+ if (match_basename(basename,
@@ dir.c: static struct path_pattern *last_matching_pattern_from_list(const char *pathname
break;
}
5: c6d17df5e5d = 4: 24bffdab139 dir: fix pattern matching on dirs
6: 3dd1d6c228c = 5: e3a749e3182 add: fail when adding an untracked sparse file
7: 15039e031e5 = 6: 2c5c834bc9f add: skip tracked paths outside sparse-checkout cone
8: 6014ac8ab9e = 7: 430ab44e4f1 add: implement the --sparse option
9: 2bd3448be5f = 8: 4f7b5cdfa36 add: update --chmod to skip sparse paths
10: 131beda1bc3 = 9: 30ec6096939 add: update --renormalize to skip sparse paths
11: 837a9314893 = 10: 99d50921ef4 rm: add --sparse option
12: cc25ce17162 = 11: 47a1444115b rm: skip sparse paths with missing SKIP_WORKTREE
13: 63a9cd80ade = 12: 28e703d80d3 mv: refuse to move sparse paths
14: 79a3518dc15 = 13: 9fbc88ee0da advice: update message to suggest '--sparse'
--
gitgitgadget
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:27
From: Derrick Stolee <redacted>
The tests in t3705-add-sparse-checkout.sh check to see how 'git add'
behaves with paths outside the sparse-checkout definition. These
currently check to see if a given warning is present but not that the
index is not updated with the sparse entries. Add a new
'test_sparse_entry_unstaged' helper to be sure 'git add' is behaving
correctly.
We need to modify setup_sparse_entry to actually commit the sparse_entry
file so it exists at HEAD and as an entry in the index, but its exact
contents are not staged in the index.
Signed-off-by: Derrick Stolee <redacted>
---
t/t3705-add-sparse-checkout.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:27
From: Derrick Stolee <redacted>
Add some tests to demonstrate the current behavior around adding files
outside of the sparse-checkout cone. Currently, untracked files are
handled differently from tracked files. A future change will make these
cases be handled the same way.
Further expand checking that a failed 'git add' does not stage changes
to the index.
Signed-off-by: Derrick Stolee <redacted>
---
t/t1092-sparse-checkout-compatibility.sh | 28 ++++++++++++++++++++++++
1 file changed, 28 insertions(+)
@@ -291,6 +301,20 @@ test_expect_success 'add, commit, checkout' 'test_all_matchgitcheckout-'+# NEEDSWORK: This documents current behavior, but is not a desirable+# behavior (untracked files are handled differently than tracked).+test_expect_success'add outside sparse cone''+init_repos&&++run_on_sparsemkdirfolder1&&+run_on_sparse../edit-contentsfolder1/a&&+run_on_sparse../edit-contentsfolder1/newfile&&+test_sparse_matchtest_must_failgitaddfolder1/a&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/a&&+test_sparse_matchgitaddfolder1/newfile+'+ test_expect_success'commit including unstaged changes''init_repos&&
@@ -339,7 +363,11 @@ test_expect_success 'status/add: outside sparse cone' '# Adding the path outside of the sparse-checkout cone should fail.test_sparse_matchtest_must_failgitaddfolder1/a&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/a&&test_sparse_matchtest_must_failgitadd--refreshfolder1/a&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/a&&# NEEDSWORK: Adding a newly-tracked file outside the cone succeedstest_sparse_matchgitaddfolder1/new&&
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:29
From: Derrick Stolee <redacted>
When matching a path against a list of patterns, the ones that require a
directory match previously did not work when a filename is specified.
This was fine when all pattern-matching was done within methods such as
unpack_trees() that check a directory before recursing into the
contained files. However, other commands will start matching individual
files against pattern lists without that recursive approach.
The last_matching_pattern_from_list() logic performs some checks on the
filetype of a path within the index when the PATTERN_FLAG_MUSTBEDIR flag
is set. This works great when setting SKIP_WORKTREE bits within
unpack_trees(), but doesn't work well when passing an arbitrary path
such as a file within a matching directory.
We extract the logic around determining the file type, but attempt to
avoid checking the filesystem if the parent directory already matches
the sparse-checkout patterns. The new path_matches_dir_pattern() method
includes a 'path_parent' parameter that is used to store the parent
directory of 'pathname' between multiple pattern matching tests. This is
loaded lazily, only on the first pattern it finds that has the
PATTERN_FLAG_MUSTBEDIR flag.
If we find that a path has a parent directory, we start by checking to
see if that parent directory matches the pattern. If so, then we do not
need to query the index for the type (which can be expensive). If we
find that the parent does not match, then we still must check the type
from the index for the given pathname.
Note that this does not affect cone mode pattern matching, but instead
the more general -- and slower -- full pattern set. Thus, this does not
affect the sparse index.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
dir.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 49 insertions(+), 5 deletions(-)
@@ -1303,6 +1303,44 @@ int match_pathname(const char *pathname, int pathlen,WM_PATHNAME)==0;}+staticintpath_matches_dir_pattern(constchar*pathname,+intpathlen,+structstrbuf**path_parent,+int*dtype,+structpath_pattern*pattern,+structindex_state*istate)+{+if(!*path_parent){+char*slash;+CALLOC_ARRAY(*path_parent,1);+strbuf_add(*path_parent,pathname,pathlen);+slash=find_last_dir_sep((*path_parent)->buf);++if(slash)+strbuf_setlen(*path_parent,slash-(*path_parent)->buf);+else+strbuf_setlen(*path_parent,0);+}++/*+*Iftheparentdirectorymatchesthepattern,thenwedonot+*needtocheckfordtype.+*/+if((*path_parent)->len&&+match_pathname((*path_parent)->buf,(*path_parent)->len,+pattern->base,+pattern->baselen?pattern->baselen-1:0,+pattern->pattern,pattern->nowildcardlen,+pattern->patternlen,pattern->flags))+return1;++*dtype=resolve_dtype(*dtype,istate,pathname,pathlen);+if(*dtype!=DT_DIR)+return0;++return1;+}+/**Scanthegivenexcludelistinreversetoseewhetherpathname*shouldbeignored.Thefirstmatch(i.e.thelastonthelist),if
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:30
From: Derrick Stolee <redacted>
Within match_pathname(), one successful matching category happens when
the pattern is equal to its non-wildcard prefix. At this point, we have
checked that the input 'pathname' matches the pattern up to the prefix
length, and then we subtraced that length from both 'patternlen' and
'namelen'.
In the case of a directory match, this prefix match should be
sufficient. However, the success condition only cared about _exact_
equality here. Instead, we should allow any path that agrees on this
prefix in the case of PATTERN_FLAG_MUSTBEDIR.
This case was not tested before because of the way unpack_trees() would
match a parent directory before visiting the contained paths. This
approach is changing, so we must change this comparison.
Signed-off-by: Derrick Stolee <redacted>
---
dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1294,7 +1294,7 @@ int match_pathname(const char *pathname, int pathlen,*thenourprefixmatchisallweneed;we*donotneedtocallfnmatchatall.*/-if(!patternlen&&!namelen)+if(!patternlen&&(!namelen||(flags&PATTERN_FLAG_MUSTBEDIR)))return1;}
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:31
From: Derrick Stolee <redacted>
The add_files() method in builtin/add.c takes a set of untracked files
that are being added by the input pathspec and inserts them into the
index. If these files are outside of the sparse-checkout cone, then they
gain the SKIP_WORKTREE bit at some point. However, this was not checked
before inserting into the index, so these files are added even though we
want to avoid modifying the index outside of the sparse-checkout cone.
Add a check within add_files() for these files and write the advice
about files outside of the sparse-checkout cone.
This behavior change modifies some existing tests within t1092. These
tests intended to document how a user could interact with the existing
behavior in place. Many of these tests need to be marked as expecting
failure. A future change will allow these tests to pass by adding a flag
to 'git add' that allows users to modify index entries outside of the
sparse-checkout cone.
The 'submodule handling' test is intended to document what happens to
directories that contain a submodule when the sparse index is enabled.
It is not trying to say that users should be able to add submodules
outside of the sparse-checkout cone, so that test can be modified to
avoid that operation.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/add.c | 14 +++++++++
t/t1092-sparse-checkout-compatibility.sh | 37 ++++++++++++++++++------
2 files changed, 42 insertions(+), 9 deletions(-)
@@ -301,8 +301,6 @@ test_expect_success 'add, commit, checkout' 'test_all_matchgitcheckout-'-# NEEDSWORK: This documents current behavior, but is not a desirable-# behavior (untracked files are handled differently than tracked). test_expect_success'add outside sparse cone''init_repos&&
@@ -312,7 +310,9 @@ test_expect_success 'add outside sparse cone' 'test_sparse_matchtest_must_failgitaddfolder1/a&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder1/a&&-test_sparse_matchgitaddfolder1/newfile+test_sparse_matchtest_must_failgitaddfolder1/newfile&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/newfile' test_expect_success'commit including unstaged changes''
@@ -343,7 +343,11 @@ test_expect_success 'commit including unstaged changes' 'test_all_matchgitstatus--porcelain=v2'-test_expect_success'status/add: outside sparse cone''+# NEEDSWORK: Now that 'git add folder1/new' fails, the changes being+# attempted here fail for the sparse-checkout and sparse-index repos.+# We must enable a way for adding files outside the sparse-checkout+# done, even if it is by an optional flag.+test_expect_failure'status/add: outside sparse cone''init_repos&&# folder1 is at HEAD, but outside the sparse cone
@@ -368,10 +372,11 @@ test_expect_success 'status/add: outside sparse cone' 'test_sparse_matchtest_must_failgitadd--refreshfolder1/a&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder1/a&&+test_sparse_matchtest_must_failgitaddfolder1/new&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/new&&-# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds-test_sparse_matchgitaddfolder1/new&&-+# NEEDSWORK: behavior begins to deviate here.test_all_matchgitadd.&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgitcommit-mfolder1/new&&
@@ -527,7 +532,7 @@ test_expect_success 'merge, cherry-pick, and rebase' '# Right now, users might be using this flow to work through conflicts,# so any solution should present advice to users who try this sequence# of commands to follow whatever new method we create.-test_expect_success'merge with conflict outside cone''+test_expect_failure'merge with conflict outside cone''init_repos&&test_all_matchgitcheckout-bmerge-tipmerge-left&&
@@ -541,12 +546,18 @@ test_expect_success 'merge with conflict outside cone' 'test_all_matchgitstatus--porcelain=v2&&# 2. Add the file with conflict markers+# NEEDSWORK: Even though the merge conflict removed the+# SKIP_WORKTREE bit from the index entry for folder1/a, we should+# warn that this is a problematic add.test_all_matchgitaddfolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and# accept conflict markers as resolved content.run_on_allmvfolder2/afolder2/z&&+# NEEDSWORK: This mode now fails, because folder2/z is+# outside of the sparse-checkout cone and does not match an+# existing index entry with the SKIP_WORKTREE bit cleared.test_all_matchgitaddfolder2&&test_all_matchgitstatus--porcelain=v2&&
@@ -555,7 +566,7 @@ test_expect_success 'merge with conflict outside cone' 'test_all_matchgitrev-parseHEAD^{tree}'-test_expect_success'cherry-pick/rebase with conflict outside cone''+test_expect_failure'cherry-pick/rebase with conflict outside cone''init_repos&&forOPERATIONincherry-pickrebase
@@ -572,11 +583,17 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' 'test_all_matchgitstatus--porcelain=v2&&# 2. Add the file with conflict markers+# NEEDSWORK: Even though the merge conflict removed the+# SKIP_WORKTREE bit from the index entry for folder1/a, we should+# warn that this is a problematic add.test_all_matchgitaddfolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and# accept conflict markers as resolved content.+# NEEDSWORK: This mode now fails, because folder2/z is+# outside of the sparse-checkout cone and does not match an+# existing index entry with the SKIP_WORKTREE bit cleared.run_on_allmvfolder2/afolder2/z&&test_all_matchgitaddfolder2&&test_all_matchgitstatus--porcelain=v2&&
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:32
From: Derrick Stolee <redacted>
When 'git add' adds a tracked file that is outside of the
sparse-checkout cone, it checks the SKIP_WORKTREE bit to see if the file
exists outside of the sparse-checkout cone. This is usually correct,
except in the case of a merge conflict outside of the cone.
Modify add_pathspec_matched_against_index() to be more careful about
paths by checking the sparse-checkout patterns in addition to the
SKIP_WORKTREE bit. This causes 'git add' to no longer allow files
outside of the cone that removed the SKIP_WORKTREE bit due to a merge
conflict.
With only this change, users will only be able to add the file after
adding the file to the sparse-checkout cone. A later change will allow
users to force adding even though the file is outside of the
sparse-checkout cone.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/add.c | 4 ++++
pathspec.c | 5 +++--
t/t1091-sparse-checkout-builtin.sh | 4 +++-
t/t1092-sparse-checkout-compatibility.sh | 19 ++++++++++++-------
t/t3705-add-sparse-checkout.sh | 12 ++++++++++++
5 files changed, 34 insertions(+), 10 deletions(-)
@@ -438,6 +438,8 @@ test_expect_success 'sparse-checkout reapply' 'test_i18ngrep"warning.*The following paths are unmerged"err&&test_path_is_filetweak/folder1/a&&+# NEEDSWORK: We are asking to update a file outside of the+# sparse-checkout cone, but this is no longer allowed.git-Ctweakaddfolder1/a&&git-Ctweaksparse-checkoutreapply2>err&&test_must_be_emptyerr&&
@@ -546,10 +546,9 @@ test_expect_failure 'merge with conflict outside cone' 'test_all_matchgitstatus--porcelain=v2&&# 2. Add the file with conflict markers-# NEEDSWORK: Even though the merge conflict removed the-# SKIP_WORKTREE bit from the index entry for folder1/a, we should-# warn that this is a problematic add.-test_all_matchgitaddfolder1/a&&+test_sparse_matchtest_must_failgitaddfolder1/a&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and
@@ -558,7 +557,9 @@ test_expect_failure 'merge with conflict outside cone' '# NEEDSWORK: This mode now fails, because folder2/z is# outside of the sparse-checkout cone and does not match an# existing index entry with the SKIP_WORKTREE bit cleared.-test_all_matchgitaddfolder2&&+test_sparse_matchtest_must_failgitaddfolder2&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder2/z&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgitmerge--continue&&
@@ -586,7 +587,9 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' '# NEEDSWORK: Even though the merge conflict removed the# SKIP_WORKTREE bit from the index entry for folder1/a, we should# warn that this is a problematic add.-test_all_matchgitaddfolder1/a&&+test_sparse_matchtest_must_failgitaddfolder1/a&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and
@@ -595,7 +598,9 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' '# outside of the sparse-checkout cone and does not match an# existing index entry with the SKIP_WORKTREE bit cleared.run_on_allmvfolder2/afolder2/z&&-test_all_matchgitaddfolder2&&+test_sparse_matchtest_must_failgitaddfolder2&&+grep"Disable or modify the sparsity rules"sparse-checkout-err&&+test_sparse_unstagedfolder2/z&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgit$OPERATION--continue&&
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:33
From: Derrick Stolee <redacted>
We previously modified 'git add' to refuse updating index entries
outside of the sparse-checkout cone. This is justified to prevent users
from accidentally getting into a confusing state when Git removes those
files from the working tree at some later point.
Unfortunately, this caused some workflows that were previously possible
to become impossible, especially around merge conflicts outside of the
sparse-checkout cone. These were documented in tests within t1092.
We now re-enable these workflows using a new '--sparse' option to 'git
add'. This allows users to signal "Yes, I do know what I'm doing with
these files," and accept the consequences of the files leaving the
worktree later.
We delay updating the advice message until implementing a similar option
in 'git rm' and 'git mv'.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-add.txt | 9 +++++++-
builtin/add.c | 12 ++++++----
t/t1092-sparse-checkout-compatibility.sh | 29 +++++++++---------------
t/t3705-add-sparse-checkout.sh | 17 +++++++++++++-
4 files changed, 43 insertions(+), 24 deletions(-)
@@ -79,6 +79,13 @@ in linkgit:gitglossary[7]. --force:: Allow adding otherwise ignored files.+--sparse::+ Allow updating index entries outside of the sparse-checkout cone.+ Normally, `git add` refuses to update index entries whose paths do+ not fit within the sparse-checkout cone, since those files might+ be removed from the working tree without warning. See+ linkgit:git-sparse-checkout[1] for more details.+ -i:: --interactive:: Add modified contents in the working tree interactively to
@@ -30,6 +30,7 @@ static int patch_interactive, add_interactive, edit_interactive;staticinttake_worktree_changes;staticintadd_renormalize;staticintpathspec_file_nul;+staticintinclude_sparse;staticconstchar*pathspec_from_file;staticintlegacy_stash_p;/* support for the scripted `git stash` */
@@ -46,7 +47,7 @@ static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)structcache_entry*ce=active_cache[i];interr;-if(ce_skip_worktree(ce))+if(!include_sparse&&ce_skip_worktree(ce))continue;if(pathspec&&!ce_path_match(&the_index,ce,pathspec,NULL))
@@ -383,6 +384,7 @@ static struct option builtin_add_options[] = {OPT_BOOL(0,"refresh",&refresh_only,N_("don't add, only refresh the index")),OPT_BOOL(0,"ignore-errors",&ignore_add_errors,N_("just skip files which cannot be added because of errors")),OPT_BOOL(0,"ignore-missing",&ignore_missing,N_("check if - even missing - files are ignored in dry run")),+OPT_BOOL(0,"sparse",&include_sparse,N_("allow updating entries outside of the sparse-checkout cone")),OPT_STRING(0,"chmod",&chmod_arg,"(+|-)x",N_("override the executable bit of the listed files")),OPT_HIDDEN_BOOL(0,"warn-embedded-repo",&warn_on_embedded_repo,
@@ -461,7 +463,8 @@ static int add_files(struct dir_struct *dir, int flags)}for(i=0;i<dir->nr;i++){-if(!path_in_sparse_checkout(dir->entries[i]->name,&the_index)){+if(!include_sparse&&+!path_in_sparse_checkout(dir->entries[i]->name,&the_index)){string_list_append(&matched_sparse_paths,dir->entries[i]->name);continue;
@@ -343,11 +343,7 @@ test_expect_success 'commit including unstaged changes' 'test_all_matchgitstatus--porcelain=v2'-# NEEDSWORK: Now that 'git add folder1/new' fails, the changes being-# attempted here fail for the sparse-checkout and sparse-index repos.-# We must enable a way for adding files outside the sparse-checkout-# done, even if it is by an optional flag.-test_expect_failure'status/add: outside sparse cone''+test_expect_success'status/add: outside sparse cone''init_repos&&# folder1 is at HEAD, but outside the sparse cone
@@ -375,15 +371,16 @@ test_expect_failure 'status/add: outside sparse cone' 'test_sparse_matchtest_must_failgitaddfolder1/new&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder1/new&&+test_sparse_matchgitadd--sparsefolder1/a&&+test_sparse_matchgitadd--sparsefolder1/new&&-# NEEDSWORK: behavior begins to deviate here.-test_all_matchgitadd.&&+test_all_matchgitadd--sparse.&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgitcommit-mfolder1/new&&test_all_matchgitrev-parseHEAD^{tree}&&run_on_all../edit-contentsfolder1/newer&&-test_all_matchgitaddfolder1/&&+test_all_matchgitadd--sparsefolder1/&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgitcommit-mfolder1/newer&&test_all_matchgitrev-parseHEAD^{tree}
@@ -527,12 +524,7 @@ test_expect_success 'merge, cherry-pick, and rebase' 'done'-# NEEDSWORK: This test is documenting current behavior, but that-# behavior can be confusing to users so there is desire to change it.-# Right now, users might be using this flow to work through conflicts,-# so any solution should present advice to users who try this sequence-# of commands to follow whatever new method we create.-test_expect_failure'merge with conflict outside cone''+test_expect_success'merge with conflict outside cone''init_repos&&test_all_matchgitcheckout-bmerge-tipmerge-left&&
@@ -549,17 +541,16 @@ test_expect_failure 'merge with conflict outside cone' 'test_sparse_matchtest_must_failgitaddfolder1/a&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder1/a&&+test_all_matchgitadd--sparsefolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and# accept conflict markers as resolved content.run_on_allmvfolder2/afolder2/z&&-# NEEDSWORK: This mode now fails, because folder2/z is-# outside of the sparse-checkout cone and does not match an-# existing index entry with the SKIP_WORKTREE bit cleared.test_sparse_matchtest_must_failgitaddfolder2&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder2/z&&+test_all_matchgitadd--sparsefolder2&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgitmerge--continue&&
@@ -567,7 +558,7 @@ test_expect_failure 'merge with conflict outside cone' 'test_all_matchgitrev-parseHEAD^{tree}'-test_expect_failure'cherry-pick/rebase with conflict outside cone''+test_expect_success'cherry-pick/rebase with conflict outside cone''init_repos&&forOPERATIONincherry-pickrebase
@@ -590,6 +581,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' 'test_sparse_matchtest_must_failgitaddfolder1/a&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder1/a&&+test_all_matchgitadd--sparsefolder1/a&&test_all_matchgitstatus--porcelain=v2&&# 3. Rename the file to another sparse filename and
@@ -601,6 +593,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' 'test_sparse_matchtest_must_failgitaddfolder2&&grep"Disable or modify the sparsity rules"sparse-checkout-err&&test_sparse_unstagedfolder2/z&&+test_all_matchgitadd--sparsefolder2&&test_all_matchgitstatus--porcelain=v2&&test_all_matchgit$OPERATION--continue&&
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:36
From: Derrick Stolee <redacted>
We added checks for path_in_sparse_checkout() to portions of 'git add'
that add warnings and prevent staging a modification, but we skipped the
--chmod mode. Update chmod_pathspec() to ignore cache entries whose path
is outside of the sparse-checkout cone (unless --sparse is provided).
Add a test in t3705.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/add.c | 4 +++-
t/t3705-add-sparse-checkout.sh | 10 +++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:37
From: Derrick Stolee <redacted>
We added checks for path_in_sparse_checkout() to portions of 'git add'
that add warnings and prevent stagins a modification, but we skipped the
--renormalize mode. Update renormalize_tracked_files() to ignore cache
entries whose path is outside of the sparse-checkout cone (unless
--sparse is provided). Add a test in t3705.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/add.c | 4 +++-
t/t3705-add-sparse-checkout.sh | 12 +++++++++++-
2 files changed, 14 insertions(+), 2 deletions(-)
@@ -154,7 +154,9 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)for(i=0;i<active_nr;i++){structcache_entry*ce=active_cache[i];-if(ce_skip_worktree(ce))+if(!include_sparse&&+(ce_skip_worktree(ce)||+!path_in_sparse_checkout(ce->name,&the_index)))continue;if(ce_stage(ce))continue;/* do not touch unmerged paths */
@@ -172,6 +172,9 @@ test_expect_success 'git add fails outside of sparse-checkout definition' 'test_must_failgitadd--chmod=+xsparse_entry&&test_sparse_entry_unstaged&&+test_must_failgitadd--renormalizesparse_entry&&+test_sparse_entry_unstaged&&+# Avoid munging CRLFs to avoid an error messagegit-ccore.autocrlf=inputadd--sparsesparse_entry2>stderr&&test_must_be_emptystderr&&
@@ -181,7 +184,14 @@ test_expect_success 'git add fails outside of sparse-checkout definition' 'gitadd--sparse--chmod=+xsparse_entry2>stderr&&test_must_be_emptystderr&&test-toolread-cache--table>actual&&-grep"^100755 blob.*sparse_entry\$"actual+grep"^100755 blob.*sparse_entry\$"actual&&++gitreset&&++# This will print a message over stderr on Windows.+gitadd--sparse--renormalizesparse_entry&&+gitstatus--porcelain>actual&&+grep"^M sparse_entry\$"actual' test_expect_success'add obeys advice.updateSparsePath''
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:38
From: Derrick Stolee <redacted>
As we did previously in 'git add', add a '--sparse' option to 'git rm'
that allows modifying paths outside of the sparse-checkout definition.
The existing checks in 'git rm' are restricted to tracked files that
have the SKIP_WORKTREE bit in the current index. Future changes will
cause 'git rm' to reject removing paths outside of the sparse-checkout
definition, even if they are untracked or do not have the SKIP_WORKTREE
bit.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-rm.txt | 6 ++++++
builtin/rm.c | 8 ++++++--
t/t3602-rm-sparse-checkout.sh | 12 ++++++++++++
3 files changed, 24 insertions(+), 2 deletions(-)
@@ -72,6 +72,12 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7]. --ignore-unmatch:: Exit with a zero status even if no files matched.+--sparse::+ Allow updating index entries outside of the sparse-checkout cone.+ Normally, `git rm` refuses to update index entries whose paths do+ not fit within the sparse-checkout cone. See+ linkgit:git-sparse-checkout[1] for more.+ -q:: --quiet:: `git rm` normally outputs one line (in the form of an `rm` command)
@@ -237,6 +237,7 @@ static int check_local_mod(struct object_id *head, int index_only)staticintshow_only=0,force=0,index_only=0,recursive=0,quiet=0;staticintignore_unmatch=0,pathspec_file_nul;+staticintinclude_sparse;staticchar*pathspec_from_file;staticstructoptionbuiltin_rm_options[]={
@@ -247,6 +248,7 @@ static struct option builtin_rm_options[] = {OPT_BOOL('r',NULL,&recursive,N_("allow recursive removal")),OPT_BOOL(0,"ignore-unmatch",&ignore_unmatch,N_("exit with a zero status even if nothing matched")),+OPT_BOOL(0,"sparse",&include_sparse,N_("allow updating entries outside of the sparse-checkout cone")),OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),OPT_END(),
@@ -322,7 +325,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)seen_any=1;elseif(ignore_unmatch)continue;-elseif(matches_skip_worktree(&pathspec,i,&skip_worktree_seen))+elseif(!include_sparse&&+matches_skip_worktree(&pathspec,i,&skip_worktree_seen))string_list_append(&only_match_skip_worktree,original);elsedie(_("pathspec '%s' did not match any files"),original);
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:39
From: Derrick Stolee <redacted>
If a path does not match the sparse-checkout cone but is somehow missing
the SKIP_WORKTREE bit, then 'git rm' currently succeeds in removing the
file. One reason a user might be in this situation is a merge conflict
outside of the sparse-checkout cone. Removing such a file might be
problematic for users who are not sure what they are doing.
Add a check to path_in_sparse_checkout() when 'git rm' is checking if a
path should be considered for deletion. Of course, this check is ignored
if the '--sparse' option is specified, allowing users who accept the
risks to continue with the removal.
This also removes a confusing behavior where a user asks for a directory
to be removed, but only the entries that are within the sparse-checkout
definition are removed. Now, 'git rm <dir>' will fail without '--sparse'
and will succeed in removing all contained paths with '--sparse'.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/rm.c | 4 +++-
t/t3602-rm-sparse-checkout.sh | 19 +++++++++++++++++--
2 files changed, 20 insertions(+), 3 deletions(-)
@@ -37,9 +37,13 @@ done test_expect_success'recursive rm does not remove sparse entries''gitreset--hard&&gitsparse-checkoutsetsub/dir&&-gitrm-rsub&&+test_must_failgitrm-rsub&&+gitrm--sparse-rsub&&gitstatus--porcelain-uno>actual&&-echo"D sub/dir/e">expected&&+cat>expected<<-\EOF&&+Dsub/d+Dsub/dir/e+EOFtest_cmpexpectedactual'
@@ -87,4 +91,15 @@ test_expect_success 'do not warn about sparse entries with --ignore-unmatch' 'gitls-files--error-unmatchb'+test_expect_success'refuse to rm a non-skip-worktree path outside sparse cone''+gitreset--hard&&+gitsparse-checkoutseta&&+gitupdate-index--no-skip-worktreeb&&+test_must_failgitrmb2>stderr&&+test_cmpb_error_and_hintstderr&&+gitrm--sparseb2>stderr&&+test_must_be_emptystderr&&+test_path_is_missingb+'+ test_done
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:41
From: Derrick Stolee <redacted>
Since cmd_mv() does not operate on cache entries and instead directly
checks the filesystem, we can only use path_in_sparse_checkout() as a
mechanism for seeing if a path is sparse or not. Be sure to skip
returning a failure if '-k' is specified.
To ensure that the advice around sparse paths is the only reason a move
failed, be sure to check this as the very last thing before inserting
into the src_for_dst list.
The tests cover a variety of cases such as whether the target is tracked
or untracked, and whether the source or destination are in or outside of
the sparse-checkout definition.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
builtin/mv.c | 52 ++++++++--
t/t7002-mv-sparse-checkout.sh | 186 ++++++++++++++++++++++++++++++++++
2 files changed, 229 insertions(+), 9 deletions(-)
create mode 100755 t/t7002-mv-sparse-checkout.sh
@@ -118,21 +118,23 @@ static int index_range_of_same_dir(const char *src, int length,intcmd_mv(intargc,constchar**argv,constchar*prefix){inti,flags,gitmodules_modified=0;-intverbose=0,show_only=0,force=0,ignore_errors=0;+intverbose=0,show_only=0,force=0,ignore_errors=0,ignore_sparse=0;structoptionbuiltin_mv_options[]={OPT__VERBOSE(&verbose,N_("be verbose")),OPT__DRY_RUN(&show_only,N_("dry run")),OPT__FORCE(&force,N_("force move/rename even if target exists"),PARSE_OPT_NOCOMPLETE),OPT_BOOL('k',NULL,&ignore_errors,N_("skip move/rename errors")),+OPT_BOOL(0,"sparse",&ignore_sparse,N_("allow updating entries outside of the sparse-checkout cone")),OPT_END(),};constchar**source,**destination,**dest_path,**submodule_gitfile;-enumupdate_mode{BOTH=0,WORKING_DIRECTORY,INDEX}*modes;+enumupdate_mode{BOTH=0,WORKING_DIRECTORY,INDEX,SPARSE}*modes;structstatst;structstring_listsrc_for_dst=STRING_LIST_INIT_NODUP;structlock_filelock_file=LOCK_INIT;structcache_entry*ce;+structstring_listonly_match_skip_worktree=STRING_LIST_INIT_NODUP;git_config(git_default_config,NULL);
@@ -176,14 +178,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)constchar*src=source[i],*dst=destination[i];intlength,src_is_dir;constchar*bad=NULL;+intskip_sparse=0;if(show_only)printf(_("Checking rename of '%s' to '%s'\n"),src,dst);length=strlen(src);-if(lstat(src,&st)<0)-bad=_("bad source");-elseif(!strncmp(src,dst,length)&&+if(lstat(src,&st)<0){+/* only error if existence is expected. */+if(modes[i]!=SPARSE)+bad=_("bad source");+}elseif(!strncmp(src,dst,length)&&(dst[length]==0||dst[length]=='/')){bad=_("can not move directory into itself");}elseif((src_is_dir=S_ISDIR(st.st_mode))
@@ -244,14 +250,36 @@ int cmd_mv(int argc, const char **argv, const char *prefix)bad=_("multiple sources for the same target");elseif(is_dir_sep(dst[strlen(dst)-1]))bad=_("destination directory does not exist");-else+else{+/*+*Wecheckifthepathsareinthesparse-checkout+*definitionasaveryfinalcheck,sincethat+*allowsustopointtheusertothe--sparse+*optionasawaytohaveasuccessfulrun.+*/+if(!ignore_sparse&&+!path_in_sparse_checkout(src,&the_index)){+string_list_append(&only_match_skip_worktree,src);+skip_sparse=1;+}+if(!ignore_sparse&&+!path_in_sparse_checkout(dst,&the_index)){+string_list_append(&only_match_skip_worktree,dst);+skip_sparse=1;+}++if(skip_sparse)+gotoremove_entry;+string_list_insert(&src_for_dst,dst);+}if(!bad)continue;if(!ignore_errors)die(_("%s, source=%s, destination=%s"),bad,src,dst);+remove_entry:if(--argc>0){intn=argc-i;memmove(source+i,source+i+1,
@@ -0,0 +1,186 @@+#!/bin/sh++test_description='git mv in sparse working trees'++../test-lib.sh++test_expect_success'setup'"+mkdir-psub/dirsub/dir2&&+touchabcsub/dsub/dir/esub/dir2/e&&+gitadd-A&&+gitcommit-mfiles&&++cat>sparse_error_header<<-EOF&&+Thefollowingpathspecsdidn'tmatchanyeligiblepath,buttheydomatchindex+entriesoutsidethecurrentsparsecheckout:+EOF++cat>sparse_hint<<-EOF+hint:Disableormodifythesparsityrulesifyouintendtoupdatesuchentries.+hint:Disablethismessagewith\"gitconfigadvice.updateSparsePathfalse\"+EOF+"++test_expect_success'mv refuses to move sparse-to-sparse''+test_when_finishedrm-fe&&+gitreset--hard&&+gitsparse-checkoutseta&&+touchb&&+test_must_failgitmvbe2>stderr&&+catsparse_error_header>expect&&+echob>>expect&&+echoe>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparsebe2>stderr&&+test_must_be_emptystderr+'++test_expect_success'mv refuses to move sparse-to-sparse, ignores failure''+test_when_finishedrm-fbce&&+gitreset--hard&&+gitsparse-checkoutseta&&++# tracked-to-untracked+touchb&&+gitmv-kbe2>stderr&&+test_path_existsb&&+test_path_is_missinge&&+catsparse_error_header>expect&&+echob>>expect&&+echoe>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&++gitmv--sparsebe2>stderr&&+test_must_be_emptystderr&&+test_path_is_missingb&&+test_path_existse&&++# tracked-to-tracked+gitreset--hard&&+touchb&&+gitmv-kbc2>stderr&&+test_path_existsb&&+test_path_is_missingc&&+catsparse_error_header>expect&&+echob>>expect&&+echoc>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&++gitmv--sparsebc2>stderr&&+test_must_be_emptystderr&&+test_path_is_missingb&&+test_path_existsc+'++test_expect_success'mv refuses to move non-sparse-to-sparse''+test_when_finishedrm-fbce&&+gitreset--hard&&+gitsparse-checkoutseta&&++# tracked-to-untracked+test_must_failgitmvae2>stderr&&+test_path_existsa&&+test_path_is_missinge&&+catsparse_error_header>expect&&+echoe>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparseae2>stderr&&+test_must_be_emptystderr&&+test_path_is_missinga&&+test_path_existse&&++# tracked-to-tracked+rme&&+gitreset--hard&&+test_must_failgitmvac2>stderr&&+test_path_existsa&&+test_path_is_missingc&&+catsparse_error_header>expect&&+echoc>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparseac2>stderr&&+test_must_be_emptystderr&&+test_path_is_missinga&&+test_path_existsc+'++test_expect_success'mv refuses to move sparse-to-non-sparse''+test_when_finishedrm-fbce&&+gitreset--hard&&+gitsparse-checkoutsetae&&++# tracked-to-untracked+touchb&&+test_must_failgitmvbe2>stderr&&+catsparse_error_header>expect&&+echob>>expect&&+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparsebe2>stderr&&+test_must_be_emptystderr+'++test_expect_success'recursive mv refuses to move (possible) sparse''+test_when_finishedrm-rfbcesub2&&+gitreset--hard&&+# Without cone mode, "sub" and "sub2" do not match+gitsparse-checkoutsetsub/dirsub2/dir&&++# Add contained contents to ensure we avoid non-existence errors+mkdirsub/dir2&&+touchsub/dsub/dir2/e&&++test_must_failgitmvsubsub22>stderr&&+catsparse_error_header>expect&&+cat>>expect<<-\EOF&&+sub/d+sub2/d+sub/dir/e+sub2/dir/e+sub/dir2/e+sub2/dir2/e+EOF+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparsesubsub22>stderr&&+test_must_be_emptystderr&&+gitcommit-m"moved sub to sub2"&&+gitrev-parseHEAD~1:sub>expect&&+gitrev-parseHEAD:sub2>actual&&+test_cmpexpectactual&&+gitreset--hardHEAD~1+'++test_expect_success'recursive mv refuses to move sparse''+gitreset--hard&&+# Use cone mode so "sub/" matches the sparse-checkout patterns+gitsparse-checkoutinit--cone&&+gitsparse-checkoutsetsub/dirsub2/dir&&++# Add contained contents to ensure we avoid non-existence errors+mkdirsub/dir2&&+touchsub/dir2/e&&++test_must_failgitmvsubsub22>stderr&&+catsparse_error_header>expect&&+cat>>expect<<-\EOF&&+sub/dir2/e+sub2/dir2/e+EOF+catsparse_hint>>expect&&+test_cmpexpectstderr&&+gitmv--sparsesubsub22>stderr&&+test_must_be_emptystderr&&+gitcommit-m"moved sub to sub2"&&+gitrev-parseHEAD~1:sub>expect&&+gitrev-parseHEAD:sub2>actual&&+test_cmpexpectactual&&+gitreset--hardHEAD~1+'++test_done
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-09-24 15:39:42
From: Derrick Stolee <redacted>
The previous changes modified the behavior of 'git add', 'git rm', and
'git mv' to not adjust paths outside the sparse-checkout cone, even if
they exist in the working tree and their cache entries lack the
SKIP_WORKTREE bit. The intention is to warn users that they are doing
something potentially dangerous. The '--sparse' option was added to each
command to allow careful users the same ability they had before.
To improve the discoverability of this new functionality, add a message
to advice.updateSparsePath that mentions the existence of the option.
The previous set of changes also modified the purpose of this message to
include possibly a list of paths instead of only a list of pathspecs.
Make the warning message more clear about this new behavior.
Signed-off-by: Derrick Stolee <redacted>
---
advice.c | 11 ++++++-----
t/t3602-rm-sparse-checkout.sh | 9 ++++++---
t/t3705-add-sparse-checkout.sh | 9 ++++++---
t/t7002-mv-sparse-checkout.sh | 9 ++++++---
4 files changed, 24 insertions(+), 14 deletions(-)
@@ -293,15 +293,16 @@ void advise_on_updating_sparse_paths(struct string_list *pathspec_list)if(!pathspec_list->nr)return;-fprintf(stderr,_("The following pathspecs didn't match any"-" eligible path, but they do match index\n"-"entries outside the current sparse checkout:\n"));+fprintf(stderr,_("The following paths and/or pathspecs matched paths that exist\n"+"outside of your sparse-checkout definition, so will not be\n"+"updated in the index:\n"));for_each_string_list_item(item,pathspec_list)fprintf(stderr,"%s\n",item->string);advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,-_("Disable or modify the sparsity rules if you intend"-" to update such entries."));+_("If you intend to update such entries, try one of the following:\n"+"* Use the --sparse option.\n"+"* Disable or modify the sparsity rules."));}voiddetach_advice(constchar*new_name)
On Fri, Sep 24, 2021 at 8:39 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
...
Updates in v4
=============
* Instead of using 'git status' and 'grep' to detect staged changes, we use
'git diff --staged'. t1092 uses an additional --diff-filter because it
tests with merge conflicts, so it needs this extra flag.
* Patches 3 and 4 are merged into the new patch 3 to avoid temporarily
having a poorly named method.
Wouldn't this be more naturally spelled as --diff-filter=u ? (Note:
lowercase 'u', not uppercase.) Then you could drop the comment too.
Other than that nit, this round looks good to me. Feel free to add a
Reviewed-by: Elijah Newren <redacted>
From: Sean Christopherson <seanjc@google.com> Date: 2021-10-18 21:28:39
On Sun, Sep 12, 2021, Derrick Stolee via GitGitGadget wrote:
This series is based on ds/mergies-with-sparse-index.
As requested, this series looks to update the behavior of git add, git rm,
and git mv when they attempt to modify paths outside of the sparse-checkout
cone. In particular, this care is expanded to not just cache entries with
the SKIP_WORKTREE bit, but also paths that do not match the sparse-checkout
definition.
I suspect something in this series broke 'git add' and friends with "odd" sparse
definitions (I haven't actually bisected). git 2.33.0 rejects attempts to add
files with the below sparse-checkout and modified files. There appears to be a
discrepancy in the query vs. checkout logic as the rejected files are checked out
in the working tree, e.g. git sees that the local file was deleted, yet will not
stage the deletion.
There's also arguably a flaw in the "advise" trigger. AFAICT, the help message
is displayed if and only if the entire path is excluded from the working tree.
In my perfect world, git would complain and advise if there are unstaged changes
for tracked files covered by the specified path.
Note, my sparse-checkout is very much the result of trial and error to get the
exact files I care about. It's entirely possible I'm doing something weird, but
at the same time git itself is obviously confused.
Thanks!
$ cat .git/info/sparse-checkout
!arch/*
!tools/arch/*
!virt/kvm/arm/*
/*
arch/.gitignore
arch/Kconfig
arch/x86
tools/arch/x86
tools/include/uapi/linux/kvm.h
!Documentation
!drivers
$ git read-tree -mu HEAD
$ rm arch/x86/kvm/x86.c
$ git commit -a
On branch x86/kvm_find_cpuid_entry_index
Your branch is up to date with 'kvm/queue'.
You are in a sparse checkout with 40% of tracked files present.
Changes not staged for commit:
(use "git add/rm <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
deleted: arch/x86/kvm/x86.c
no changes added to commit (use "git add" and/or "git commit -a")
$ git add arch
$ git add .
$ git add arch/x86
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
arch/x86
hint: If you intend to update such entries, try one of the following:
hint: * Use the --sparse option.
hint: * Disable or modify the sparsity rules.
hint: Disable this message with "git config advice.updateSparsePath false"
On Sun, Sep 12, 2021, Derrick Stolee via GitGitGadget wrote:
quoted
This series is based on ds/mergies-with-sparse-index.
As requested, this series looks to update the behavior of git add, git rm,
and git mv when they attempt to modify paths outside of the sparse-checkout
cone. In particular, this care is expanded to not just cache entries with
the SKIP_WORKTREE bit, but also paths that do not match the sparse-checkout
definition.
I suspect something in this series broke 'git add' and friends with "odd" sparse
definitions (I haven't actually bisected). git 2.33.0 rejects attempts to add
files with the below sparse-checkout and modified files. There appears to be a
discrepancy in the query vs. checkout logic as the rejected files are checked out
in the working tree, e.g. git sees that the local file was deleted, yet will not
stage the deletion.
Are you using v2.33.0? This change is not in that version.
However, mt/add-rm-in-sparse-checkout [1] was introduced in v2.33.0 and
introduced these advice suggestions.
[1] https://github.com/git/git/compare/a5828ae6b52137b913b978e16cd2334482eb4c1f...d5f4b8260f623d6fdef36d5eaa8a0c2350390472
The series you are commenting on goes even farther in restricting adds to
be within the sparse-checkout definitions, even for unstaged files or files
that removed the skip-worktree bit due to a merge conflict. It also creates
an override '--sparse' option that allows you to ignore these protections.
There's also arguably a flaw in the "advise" trigger. AFAICT, the help message
is displayed if and only if the entire path is excluded from the working tree.
In my perfect world, git would complain and advise if there are unstaged changes
for tracked files covered by the specified path.
quoted
Note, my sparse-checkout is very much the result of trial and error to get the
exact files I care about. It's entirely possible I'm doing something weird, but
at the same time git itself is obviously confused.
Thanks!
$ cat .git/info/sparse-checkout
!arch/*
!tools/arch/*
!virt/kvm/arm/*
/*
arch/.gitignore
arch/Kconfig
arch/x86
tools/arch/x86
tools/include/uapi/linux/kvm.h
!Documentation
!drivers
Have you tried using 'arch/x86/' and 'tools/arch/x86/' to specify
that these are directories? Just a thought.
$ git read-tree -mu HEAD
$ rm arch/x86/kvm/x86.c
$ git commit -a
...
deleted: arch/x86/kvm/x86.c
This is certainly odd. Worth more investigation that I don't have
time for at this moment.
Thanks,
-Stolee
From: Sean Christopherson <seanjc@google.com> Date: 2021-10-19 16:50:16
On Tue, Oct 19, 2021, Derrick Stolee wrote:
On 10/18/2021 5:28 PM, Sean Christopherson wrote:
quoted
On Sun, Sep 12, 2021, Derrick Stolee via GitGitGadget wrote:
quoted
This series is based on ds/mergies-with-sparse-index.
As requested, this series looks to update the behavior of git add, git rm,
and git mv when they attempt to modify paths outside of the sparse-checkout
cone. In particular, this care is expanded to not just cache entries with
the SKIP_WORKTREE bit, but also paths that do not match the sparse-checkout
definition.
I suspect something in this series broke 'git add' and friends with "odd" sparse
definitions (I haven't actually bisected). git 2.33.0 rejects attempts to add
files with the below sparse-checkout and modified files. There appears to be a
discrepancy in the query vs. checkout logic as the rejected files are checked out
in the working tree, e.g. git sees that the local file was deleted, yet will not
stage the deletion.
Are you using v2.33.0? This change is not in that version.
Hrm, it's an internal build that says v2.33.0 is the bsae, but the --sparse option
is available so who knows what's actually underneath the hood. I can try vanilla
upstream builds if that would help narrow down the issue.
However, mt/add-rm-in-sparse-checkout [1] was introduced in v2.33.0 and
introduced these advice suggestions.
[1] https://github.com/git/git/compare/a5828ae6b52137b913b978e16cd2334482eb4c1f...d5f4b8260f623d6fdef36d5eaa8a0c2350390472
The series you are commenting on goes even farther in restricting adds to
be within the sparse-checkout definitions, even for unstaged files or files
that removed the skip-worktree bit due to a merge conflict. It also creates
an override '--sparse' option that allows you to ignore these protections.
quoted
There's also arguably a flaw in the "advise" trigger. AFAICT, the help message
is displayed if and only if the entire path is excluded from the working tree.
In my perfect world, git would complain and advise if there are unstaged changes
for tracked files covered by the specified path.
quoted
Note, my sparse-checkout is very much the result of trial and error to get the
exact files I care about. It's entirely possible I'm doing something weird, but
at the same time git itself is obviously confused.
Thanks!
$ cat .git/info/sparse-checkout
!arch/*
!tools/arch/*
!virt/kvm/arm/*
/*
arch/.gitignore
arch/Kconfig
arch/x86
tools/arch/x86
tools/include/uapi/linux/kvm.h
!Documentation
!drivers
Have you tried using 'arch/x86/' and 'tools/arch/x86/' to specify
that these are directories? Just a thought.
Nice! That workaround resolves the issue. I vaguely recall intentionally omitting
the trailing slash, but adding it back doesn't seem to have any unwanted side effects
on the current git versions I'm using.
quoted
$ git read-tree -mu HEAD
$ rm arch/x86/kvm/x86.c
$ git commit -a
...
quoted
deleted: arch/x86/kvm/x86.c
This is certainly odd. Worth more investigation that I don't have
time for at this moment.
I've no objection to punting on this now that I have a workaround. The man pages
are quite clear that sparse checkouts are still experimental and it's no trouble
for me to whine again if something breaks in the future :-)
Thanks again!
From: Junio C Hamano <hidden> Date: 2021-10-20 13:28:10
Sean Christopherson [off-list ref] writes:
quoted
Are you using v2.33.0? This change is not in that version.
Hrm, it's an internal build that says v2.33.0 is the bsae, but the --sparse option
is available so who knows what's actually underneath the hood. I can try vanilla
upstream builds if that would help narrow down the issue.
$ git version
Guessing from the e-mail address, perhaps you are using something
derived from the next branch of the day, maintained by Jonathan
Nieder's group, for internal consumption at Google.
From: Sean Christopherson <seanjc@google.com> Date: 2021-10-20 14:29:19
On Wed, Oct 20, 2021, Junio C Hamano wrote:
Sean Christopherson [off-list ref] writes:
quoted
quoted
Are you using v2.33.0? This change is not in that version.
Hrm, it's an internal build that says v2.33.0 is the bsae, but the --sparse option
is available so who knows what's actually underneath the hood. I can try vanilla
upstream builds if that would help narrow down the issue.
$ git version
Guessing from the e-mail address, perhaps you are using something
derived from the next branch of the day, maintained by Jonathan
Nieder's group, for internal consumption at Google.
That's more than likely the case. 2.33.0.1079.g6e70778dc9-goog
$ git add arch/x86
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
arch/x86
I think the problem may be that we are performing pattern matching
slightly different in add, mv, and rm, in comparison to "git
sparse-checkout". On "git sparse-checkout init" (or reapply), we call
clear_ce_flags() which calls path_matches_pattern_list() for each
component of the working tree paths. If the full path gives a match
result of UNDECIDED, we recursively try to use the match result from
the parent dir (or NOT_MATCHED if we reach the top with UNDECIDED).
In Sean's example, we get UNDECIDED for "arch/x86/kvm/x86.c", but
"arch/x86" gives MATCHED, so we end up using that for the full path.
However, in add|mv|rm we only call path_matches_pattern_list() for the
full path and get UNDECIDED, which we consider the same as NOT_MATCHED,
and end up disallowing the path update operation with a warning message.
The commands do work if we replace the sparsity pattern "arch/x86" with
"arch/x86/" (with a trailing slash), but note that it only works
because the pattern is relative to the root (see dir.c:1297). If we
change it to "x86/", it would no longer work.
So far, the only way I could think of to fix this would be to perform
pattern matching for the leading components of the paths too. That
doesn't seem very nice, though, as it can probably be quite expensive...
But here is a patch for discussion:
-- >8 --
Subject: [RFC PATCH] add|rm|mv: fix bug that prevent the update of non-sparse dirs
These three commands recently learned to avoid updating paths that do
not match the sparse-checkout patterns even if they are missing the
SKIP_WORKTREE bit. This is done using path_in_sparse_checkout(), which
tries to match the path with the current set of sparsity rules using
path_matches_pattern_list(). This is similar to what clear_ce_flags()
does when we run "git sparse-checkout init" or "git sparse-checkout
reapply". But note that clear_ce_flags() has a recursive behavior,
calling path_matches_pattern_list() for each component in a path,
whereas path_in_sparse_checkout() only calls it for the full path. This
makes the function miss matches such as the one between path "a/b/c" and
the pattern "b/". So if the user has the sparsity rules "!/a" and "b/",
for example, add, rm, and mv will fail to update the path "a/b/c" and
end up displaying a warning about "a/b/c" being outside the sparse
checkout even though it isn't. Note that this problem only occurs with
non-cone mode.
Fix this by making path_in_sparse_checkout() perform pattern matching
for every component in the given path when cone mode is disabled. (This
can be expensive, and we might want to do some form of caching for the
match results of the leading components. However, this is not
implemented in this patch.) Also add two tests for each command (add,
rm, and mv) to check that they behave correctly with the said pattern
matching. The first test would previously fail without this patch, while
the second already succeeded. It is added mostly to make sure that we
are not breaking the existing pattern matching for directories that are
really sparse, and also as a protection against any future
regressions.
Note that two other existing tests had to be changed: one test in t3602
checks that "git rm -r <dir>" won't remove sparse entries, but it
didn't allow the non-sparse entries inside <dir> to be removed. The
other one, in t7002, tested that "git mv" would correctly display a
warning message for sparse paths, but it accidentally expected the
message to include two non-sparse paths as well.
Signed-off-by: Matheus Tavares <redacted>
---
dir.c | 33 ++++++++++++++++++++++++------
t/t3602-rm-sparse-checkout.sh | 37 +++++++++++++++++++++++++++++++---
t/t3705-add-sparse-checkout.sh | 18 +++++++++++++++++
t/t7002-mv-sparse-checkout.sh | 28 +++++++++++++++++++++++--
4 files changed, 105 insertions(+), 11 deletions(-)
@@ -1516,11 +1517,31 @@ static int path_in_sparse_checkout_1(const char *path,!istate->sparse_checkout_patterns->use_cone_patterns))return1;-base=strrchr(path,'/');-returnpath_matches_pattern_list(path,strlen(path),base?base+1:path,-&dtype,-istate->sparse_checkout_patterns,-istate)>0;+if(istate->sparse_checkout_patterns->use_cone_patterns){+constchar*base=strrchr(path,'/');+returnpath_matches_pattern_list(path,strlen(path),+base?base+1:path,&dtype,+istate->sparse_checkout_patterns,istate)>0;+}++for(p=path;;p++){+enumpattern_match_resultmatch;++if(*p&&*p!='/')+continue;++match=path_matches_pattern_list(path,p-path,+last_slash?last_slash+1:path,&dtype,+istate->sparse_checkout_patterns,istate);++if(match!=UNDECIDED)+ret=match;+if(!*p)+break;+last_slash=p;+}++returnret;}
Of course, after hitting send I realized it would make a lot more sense
to start the pattern matching from the full path and only go backwards
through the parent dirs until we find the first non-UNDECIDED result.
I.e. something like this:
static int path_in_sparse_checkout_1(const char *path,
struct index_state *istate,
int require_cone_mode)
{
int dtype = DT_REG;
enum pattern_match_result ret;
const char *p, *base;
/*
* We default to accepting a path if there are no patterns or
* they are of the wrong type.
*/
if (init_sparse_checkout_patterns(istate) ||
(require_cone_mode &&
!istate->sparse_checkout_patterns->use_cone_patterns))
return 1;
if (istate->sparse_checkout_patterns->use_cone_patterns) {
base = strrchr(path, '/');
return path_matches_pattern_list(path, strlen(path),
base ? base + 1 : path, &dtype,
istate->sparse_checkout_patterns, istate) > 0;
}
/*
* If the match for the path is UNDECIDED, try to match the parent dir
* recursively.
*/
for (p = path + strlen(path); p && p > path; p = base) {
base = memrchr(path, '/', p - path);
ret = path_matches_pattern_list(path, p - path,
base ? base + 1 : path, &dtype,
istate->sparse_checkout_patterns, istate);
if (ret != UNDECIDED)
break;
}
return ret == UNDECIDED ? NOT_MATCHED : ret;
}
But I will let others comment on the overall idea and/or other
alternatives before sending a possible v2.
$ git add arch/x86
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
arch/x86
I think the problem may be that we are performing pattern matching
slightly different in add, mv, and rm, in comparison to "git
sparse-checkout". On "git sparse-checkout init" (or reapply), we call
clear_ce_flags() which calls path_matches_pattern_list() for each
component of the working tree paths. If the full path gives a match
result of UNDECIDED, we recursively try to use the match result from
the parent dir (or NOT_MATCHED if we reach the top with UNDECIDED).
Yes! I think this is absolutely the problem. Thanks for pointing
this out!
In Sean's example, we get UNDECIDED for "arch/x86/kvm/x86.c", but
"arch/x86" gives MATCHED, so we end up using that for the full path.
However, in add|mv|rm we only call path_matches_pattern_list() for the
full path and get UNDECIDED, which we consider the same as NOT_MATCHED,
and end up disallowing the path update operation with a warning message.
The commands do work if we replace the sparsity pattern "arch/x86" with
"arch/x86/" (with a trailing slash), but note that it only works
because the pattern is relative to the root (see dir.c:1297). If we
change it to "x86/", it would no longer work.
So far, the only way I could think of to fix this would be to perform
pattern matching for the leading components of the paths too. That
doesn't seem very nice, though, as it can probably be quite expensive...
But here is a patch for discussion:
I agree that it is expensive, but that's already the case for the
non-cone sparse-checkout patterns. Hopefully it is sufficient that
these cases are restricted to modified files (in the case of `git add .`)
or specific pathspecs (in the case of `git mv` and `git rm`).
-- >8 --
Subject: [RFC PATCH] add|rm|mv: fix bug that prevent the update of non-sparse dirs
These three commands recently learned to avoid updating paths that do
not match the sparse-checkout patterns even if they are missing the
SKIP_WORKTREE bit. This is done using path_in_sparse_checkout(), which
tries to match the path with the current set of sparsity rules using
path_matches_pattern_list(). This is similar to what clear_ce_flags()
does when we run "git sparse-checkout init" or "git sparse-checkout
reapply". But note that clear_ce_flags() has a recursive behavior,
calling path_matches_pattern_list() for each component in a path,
whereas path_in_sparse_checkout() only calls it for the full path. This
makes the function miss matches such as the one between path "a/b/c" and
the pattern "b/". So if the user has the sparsity rules "!/a" and "b/",
for example, add, rm, and mv will fail to update the path "a/b/c" and
end up displaying a warning about "a/b/c" being outside the sparse
checkout even though it isn't. Note that this problem only occurs with
non-cone mode.
Fix this by making path_in_sparse_checkout() perform pattern matching
for every component in the given path when cone mode is disabled. (This
can be expensive, and we might want to do some form of caching for the
match results of the leading components. However, this is not
implemented in this patch.) Also add two tests for each command (add,
rm, and mv) to check that they behave correctly with the said pattern
matching. The first test would previously fail without this patch, while
the second already succeeded. It is added mostly to make sure that we
are not breaking the existing pattern matching for directories that are
really sparse, and also as a protection against any future
regressions.
Note that two other existing tests had to be changed: one test in t3602
checks that "git rm -r <dir>" won't remove sparse entries, but it
didn't allow the non-sparse entries inside <dir> to be removed. The
other one, in t7002, tested that "git mv" would correctly display a
warning message for sparse paths, but it accidentally expected the
message to include two non-sparse paths as well.
quoted hunk
@@ -1504,8 +1504,9 @@ static int path_in_sparse_checkout_1(const char *path, struct index_state *istate, int require_cone_mode) {- const char *base; int dtype = DT_REG;+ enum pattern_match_result ret = NOT_MATCHED;+ const char *p, *last_slash = NULL; /* * We default to accepting a path if there are no patterns or
@@ -1516,11 +1517,31 @@ static int path_in_sparse_checkout_1(const char *path, !istate->sparse_checkout_patterns->use_cone_patterns)) return 1;- base = strrchr(path, '/');- return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,- &dtype,- istate->sparse_checkout_patterns,- istate) > 0;+ if (istate->sparse_checkout_patterns->use_cone_patterns) {+ const char *base = strrchr(path, '/');+ return path_matches_pattern_list(path, strlen(path),+ base ? base + 1 : path, &dtype,+ istate->sparse_checkout_patterns, istate) > 0;+ }++ for (p = path; ; p++) {+ enum pattern_match_result match;++ if (*p && *p != '/')+ continue;++ match = path_matches_pattern_list(path, p - path,+ last_slash ? last_slash + 1 : path, &dtype,+ istate->sparse_checkout_patterns, istate);++ if (match != UNDECIDED)+ ret = match;+ if (!*p)+ break;+ last_slash = p;+ }++ return ret;
This implementation makes sense to me.
test_expect_success 'recursive rm does not remove sparse entries' '
git reset --hard &&
git sparse-checkout set sub/dir &&
- test_must_fail git rm -r sub &&
- git rm --sparse -r sub &&
+ git rm -r sub &&
Interesting that the new pattern-matching already presents a change of
behavior in this test case.
git status --porcelain -uno >actual &&
cat >expected <<-\EOF &&
+ D sub/dir/e
+ EOF
+ test_cmp expected actual &&
And here is why. Excellent. I suppose that setting the pattern to be
"sub/dir/" would have shown this behavior before.
+
+ git rm --sparse -r sub &&
+ git status --porcelain -uno >actual2 &&
+ cat >expected2 <<-\EOF &&
D sub/d
D sub/dir/e
EOF
- test_cmp expected actual
+ test_cmp expected2 actual2
'
The rest of the test cases add new checks that are very valuable.
I love this idea and I agree that it would be better to change the
loop direction to match the full path first (as you mention in your
response).
Thanks,
-Stolee
This patch changes the behavior of .gitignore such that directories are
now matched by prefix instead of matching exactly.
The failure that we observed is something like the following:
In "a/.gitignore", we have the pattern "git/". We should expect that
"a/git/foo" to be ignored because "git/" should be matched exactly.
However, "a/git-foo/bar" is also ignored because "git-foo" matches the
prefix.
I'll prepare a test case for this as soon as I figure out how to write
it..