| Message ID | e80fcfa932cca394c5c8b349cafadb0754a594dd.1629842085.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior | expand |
On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > 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 sprase-checkout cone. s/sprase/sparse/ > 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. While I was playing with this patch, I did the following: echo a >a echo b >b git add . git commit -m files git sparse-checkout set a echo c >c git add c And the last `git add` was successful in adding the untracked `c` file which is outside the sparse checkout. I'm not sure if I'm doing something wrong, but it seems that `path_in_sparse_checkout()` returns UNDECIDED for `c`. Is it because there was no pattern in the list explicitly excluding it? And if so, should we consider UNDECIDED as NOT_MATCHED for `path_in_sparse_checkout()`? > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/add.c | 14 ++++++++++ > t/t1092-sparse-checkout-compatibility.sh | 33 +++++++++++++++++------- > 2 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index 88a6c0c69fb..3a109276b74 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -443,6 +443,7 @@ static void check_embedded_repo(const char *path) > static int add_files(struct dir_struct *dir, int flags) > { > int i, exit_status = 0; > + struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; I see this reuses the logic from cmd_add() and refresh(). But since we are operating on untracked files here, perhaps we could replace "skip_worktree" by "sparse_paths" or something similar? > if (dir->ignored_nr) { > fprintf(stderr, _(ignore_error)); > @@ -456,6 +457,11 @@ 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)) { > + string_list_append(&only_match_skip_worktree, > + dir->entries[i]->name); > + continue; > + } > if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) { > if (!ignore_add_errors) > die(_("adding files failed")); > @@ -464,6 +470,14 @@ static int add_files(struct dir_struct *dir, int flags) > check_embedded_repo(dir->entries[i]->name); > } > } > + > + if (only_match_skip_worktree.nr) { > + advise_on_updating_sparse_paths(&only_match_skip_worktree); Hmm, advise_on_updating_sparse_paths() takes a list of pathspecs that only matched sparse paths, but here we are passing a list of actual pathnames... Well, these are technically pathspecs too, but the advice message may be confusing. For example, if we ran `git add *.c` on a repo with the untracked files `d1/file.c` and `d2/file.c`, we will get: The following pathspecs didn't match any eligible path, but they do match index entries outside the current sparse checkout: d1/file.c d2/file.c However, `d1/file.c` and `d2/file.c` are neither index entries nor the pathspecs that the user has given to `git add`. So perhaps we need to change the error/advice message? > + exit_status = 1; > + } > + string_list_clear(&only_match_skip_worktree, 0); > + > return exit_status; > }
On Fri, Aug 27, 2021 at 6:06 PM Matheus Tavares Bernardino <matheus.bernardino@usp.br> wrote: > > While I was playing with this patch, I did the following: > > echo a >a > echo b >b > git add . > git commit -m files > git sparse-checkout set a > echo c >c > git add c > > And the last `git add` was successful in adding the untracked `c` file > which is outside the sparse checkout. I'm not sure if I'm doing > something wrong, but it seems that `path_in_sparse_checkout()` returns > UNDECIDED for `c`. Is it because there was no pattern in the list > explicitly excluding it? And if so, should we consider UNDECIDED as > NOT_MATCHED for `path_in_sparse_checkout()`? Please disconsider this, It was my fault indeed. I had applied the patches onto the wrong base. Now I fetched them again but from the GGG tag, and my manual test worked as expected.
On 8/27/2021 5:06 PM, Matheus Tavares Bernardino wrote: > On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: Thanks for adding your review. I'm sorry I'm so late getting back to it. >> From: Derrick Stolee <dstolee@microsoft.com> >> >> 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 sprase-checkout cone. > > s/sprase/sparse/ Thanks. >> diff --git a/builtin/add.c b/builtin/add.c >> index 88a6c0c69fb..3a109276b74 100644 >> --- a/builtin/add.c >> +++ b/builtin/add.c >> @@ -443,6 +443,7 @@ static void check_embedded_repo(const char *path) >> static int add_files(struct dir_struct *dir, int flags) >> { >> int i, exit_status = 0; >> + struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; > > I see this reuses the logic from cmd_add() and refresh(). But since we > are operating on untracked files here, perhaps we could replace > "skip_worktree" by "sparse_paths" or something similar? How about "matched_sparse_paths" as a whole name swap? The earlier uses cared only if every match was sparse, but here we are actually looking at cases that are untracked, and the pathspec could also match other non-sparse cases. >> + >> + if (only_match_skip_worktree.nr) { >> + advise_on_updating_sparse_paths(&only_match_skip_worktree); > > > Hmm, advise_on_updating_sparse_paths() takes a list of pathspecs that > only matched sparse paths, but here we are passing a list of actual > pathnames... Well, these are technically pathspecs too, but the advice > message may be confusing. > > For example, if we ran `git add *.c` on a repo with the untracked > files `d1/file.c` and `d2/file.c`, we will get: > > The following pathspecs didn't match any eligible path, but they do match index > entries outside the current sparse checkout: > d1/file.c > d2/file.c > > However, `d1/file.c` and `d2/file.c` are neither index entries nor the > pathspecs that the user has given to `git add`. So perhaps we need to > change the error/advice message? I think the advice should be modified to refer to paths and/or pathspecs, and also it is not completely correct anymore. Instead of The following pathspecs didn't match any eligible path, but they do match index entries outside the current sparse checkout: perhaps The following paths and/or pathspecs matched paths that exist outside of your sparse-checkout definition, so will not be updated in the index: I'm going to save this one for a new patch at the end to make sure it handles all of the cases involved in this series. Thanks, -Stolee
diff --git a/builtin/add.c b/builtin/add.c index 88a6c0c69fb..3a109276b74 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -443,6 +443,7 @@ static void check_embedded_repo(const char *path) static int add_files(struct dir_struct *dir, int flags) { int i, exit_status = 0; + struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; if (dir->ignored_nr) { fprintf(stderr, _(ignore_error)); @@ -456,6 +457,11 @@ 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)) { + string_list_append(&only_match_skip_worktree, + dir->entries[i]->name); + continue; + } if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) { if (!ignore_add_errors) die(_("adding files failed")); @@ -464,6 +470,14 @@ static int add_files(struct dir_struct *dir, int flags) check_embedded_repo(dir->entries[i]->name); } } + + if (only_match_skip_worktree.nr) { + advise_on_updating_sparse_paths(&only_match_skip_worktree); + exit_status = 1; + } + + string_list_clear(&only_match_skip_worktree, 0); + return exit_status; } diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 23bee918260..962bece03e1 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -291,8 +291,6 @@ test_expect_success 'add, commit, checkout' ' test_all_match git checkout - ' -# 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 && @@ -300,7 +298,7 @@ test_expect_success 'add outside sparse cone' ' run_on_sparse ../edit-contents folder1/a && run_on_sparse ../edit-contents folder1/newfile && test_sparse_match test_must_fail git add folder1/a && - test_sparse_match git add folder1/newfile + test_sparse_match test_must_fail git add folder1/newfile ' test_expect_success 'commit including unstaged changes' ' @@ -331,7 +329,11 @@ test_expect_success 'commit including unstaged changes' ' test_all_match git status --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 @@ -352,10 +354,9 @@ test_expect_success 'status/add: outside sparse cone' ' # Adding the path outside of the sparse-checkout cone should fail. test_sparse_match test_must_fail git add folder1/a && test_sparse_match test_must_fail git add --refresh folder1/a && + test_sparse_match test_must_fail git add folder1/new && - # NEEDSWORK: Adding a newly-tracked file outside the cone succeeds - test_sparse_match git add folder1/new && - + # NEEDSWORK: behavior begins to deviate here. test_all_match git add . && test_all_match git status --porcelain=v2 && test_all_match git commit -m folder1/new && @@ -511,7 +512,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_match git checkout -b merge-tip merge-left && @@ -525,12 +526,18 @@ test_expect_success 'merge with conflict outside cone' ' test_all_match git status --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_match git add folder1/a && test_all_match git status --porcelain=v2 && # 3. Rename the file to another sparse filename and # accept conflict markers as resolved content. run_on_all mv folder2/a folder2/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_match git add folder2 && test_all_match git status --porcelain=v2 && @@ -539,7 +546,7 @@ test_expect_success 'merge with conflict outside cone' ' test_all_match git rev-parse HEAD^{tree} ' -test_expect_success 'cherry-pick/rebase with conflict outside cone' ' +test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' init_repos && for OPERATION in cherry-pick rebase @@ -556,11 +563,17 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' ' test_all_match git status --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_match git add folder1/a && test_all_match git status --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_all mv folder2/a folder2/z && test_all_match git add folder2 && test_all_match git status --porcelain=v2 && @@ -638,6 +651,7 @@ test_expect_success 'clean' ' test_expect_success 'submodule handling' ' init_repos && + test_sparse_match git sparse-checkout add modules && test_all_match mkdir modules && test_all_match touch modules/a && test_all_match git add modules && @@ -647,6 +661,7 @@ test_expect_success 'submodule handling' ' test_all_match git commit -m "add submodule" && # having a submodule prevents "modules" from collapse + test_sparse_match git sparse-checkout set deep/deeper1 && test-tool -C sparse-index read-cache --table >cache && grep "100644 blob .* modules/a" cache && grep "160000 commit $(git -C initial-repo rev-parse HEAD) modules/sub" cache