| Message ID | 944ae2cffa8ff175cd1cee0b3a25060ec5599973.1631453010.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior | expand |
On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > 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 <dstolee@microsoft.com> > --- > Documentation/git-add.txt | 9 +++++++- > builtin/add.c | 12 +++++++---- > t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++---------------- > t/t3705-add-sparse-checkout.sh | 17 ++++++++++++++- > 4 files changed, 41 insertions(+), 24 deletions(-) > > diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt > index be5e3ac54b8..bb79016d2ca 100644 > --- a/Documentation/git-add.txt > +++ b/Documentation/git-add.txt > @@ -9,7 +9,7 @@ SYNOPSIS > -------- > [verse] > 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p] > - [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]] > + [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]] [--sparse] > [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] > [--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]] > [--] [<pathspec>...] > @@ -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. for more ...? details? I find the last sentence incomplete. Following that track for a moment and thinking out loud, I wonder if we want more details somewhere in the sparse-checkout docs about this issue and if so, if we should point to that specific part of that page. The 'reapply' section of the sparse-checkout page kind of touches on the topic of the worktree not exactly matching sparsity patterns (due to other commands), but focuses on unsparsifying files due to conflicts and kind of ignores the re-sparsification that happens in other commands after the working copy matches the index. (Such a documentation improvement could come after your series, as I said, I'm just thinking out loud.) > + > -i:: > --interactive:: > Add modified contents in the working tree interactively to > diff --git a/builtin/add.c b/builtin/add.c > index 09c3fad6321..f8e3930608d 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -30,6 +30,7 @@ static int patch_interactive, add_interactive, edit_interactive; > static int take_worktree_changes; > static int add_renormalize; > static int pathspec_file_nul; > +static int include_sparse; > static const char *pathspec_from_file; > static int legacy_stash_p; /* support for the scripted `git stash` */ > > @@ -46,7 +47,7 @@ static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only) > struct cache_entry *ce = active_cache[i]; > int err; > > - if (ce_skip_worktree(ce)) > + if (!include_sparse && ce_skip_worktree(ce)) > continue; > > if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL)) > @@ -95,7 +96,7 @@ static void update_callback(struct diff_queue_struct *q, > struct diff_filepair *p = q->queue[i]; > const char *path = p->one->path; > > - if (!path_in_sparse_checkout(path, &the_index)) > + if (!include_sparse && !path_in_sparse_checkout(path, &the_index)) > continue; > > switch (fix_unmerged_status(p, data)) { > @@ -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; > @@ -646,7 +649,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) > if (seen[i]) > continue; > > - if (matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) { > + if (!include_sparse && > + matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) { > string_list_append(&only_match_skip_worktree, > pathspec.items[i].original); > continue; > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 0fdc5c7098c..7d64d9deb22 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -343,11 +343,7 @@ test_expect_success 'commit including unstaged changes' ' > test_all_match git status --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_match test_must_fail git add folder1/new && > test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && > test_sparse_unstaged folder1/new && > + test_sparse_match git add --sparse folder1/a && > + test_sparse_match git add --sparse folder1/new && > > - # NEEDSWORK: behavior begins to deviate here. > - test_all_match git add . && > + test_all_match git add --sparse . && > test_all_match git status --porcelain=v2 && > test_all_match git commit -m folder1/new && > test_all_match git rev-parse HEAD^{tree} && > > run_on_all ../edit-contents folder1/newer && > - test_all_match git add folder1/ && > + test_all_match git add --sparse folder1/ && > test_all_match git status --porcelain=v2 && > test_all_match git commit -m folder1/newer && > test_all_match git rev-parse HEAD^{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' ' Based on the comments on the next hunk, I also wonder if this hunk doesn't belong in the previous commit... > init_repos && > > test_all_match git checkout -b merge-tip merge-left && > @@ -555,9 +547,6 @@ test_expect_failure 'merge with conflict outside cone' ' > # 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_sparse_match test_must_fail git add folder2 && > test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && > test_sparse_unstaged folder2/z && Is this hunk in the wrong commit? You added a --sparse flag to the git add a few lines below in the previous commit, so it seems the NEEDSWORK comment should have been removed at the same time. > @@ -569,7 +558,7 @@ test_expect_failure 'merge with conflict outside cone' ' > test_all_match git rev-parse HEAD^{tree} > ' > > -test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' > +test_expect_success 'cherry-pick/rebase with conflict outside cone' ' > init_repos && > > for OPERATION in cherry-pick rebase > @@ -592,6 +581,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' > test_sparse_match test_must_fail git add folder1/a && > test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && > test_sparse_unstaged folder1/a && > + test_all_match git add --sparse folder1/a && > test_all_match git status --porcelain=v2 && > > # 3. Rename the file to another sparse filename and > @@ -603,6 +593,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' > test_sparse_match test_must_fail git add folder2 && > test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && > test_sparse_unstaged folder2/z && > + test_all_match git add --sparse folder2 && > test_all_match git status --porcelain=v2 && > > test_all_match git $OPERATION --continue && > diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh > index 678be1acbf9..0f7e03b5326 100755 > --- a/t/t3705-add-sparse-checkout.sh > +++ b/t/t3705-add-sparse-checkout.sh > @@ -167,7 +167,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' ' > > git update-index --no-skip-worktree sparse_entry && > test_must_fail git add sparse_entry && > - test_sparse_entry_unstaged > + test_sparse_entry_unstaged && > + > + # Avoid munging CRLFs to avoid an error message > + git -c core.autocrlf=input add --sparse sparse_entry 2>stderr && > + test_must_be_empty stderr && > + test-tool read-cache --table >actual && > + grep "^100644 blob.*sparse_entry\$" actual Does this CRLF anti-munging belong in a separate commit somewhere? I was surprised to see it, and it's not clear to me how it relates to the other changes. Am I missing something? > ' > > test_expect_success 'add obeys advice.updateSparsePath' ' > @@ -178,4 +184,13 @@ test_expect_success 'add obeys advice.updateSparsePath' ' > > ' > > +test_expect_success 'add allows sparse entries with --sparse' ' > + git sparse-checkout set a && > + echo modified >sparse_entry && > + test_must_fail git add sparse_entry && > + test_sparse_entry_unchanged && > + git add --sparse sparse_entry 2>stderr && > + test_must_be_empty stderr > +' > + > test_done > -- > gitgitgadget
On 9/15/2021 12:59 PM, Elijah Newren wrote: > On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget ... >> +--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. > > for more ...? details? I find the last sentence incomplete. I'll add "details" > Following that track for a moment and thinking out loud, I wonder if > we want more details somewhere in the sparse-checkout docs about this > issue and if so, if we should point to that specific part of that > page. The 'reapply' section of the sparse-checkout page kind of > touches on the topic of the worktree not exactly matching sparsity > patterns (due to other commands), but focuses on unsparsifying files > due to conflicts and kind of ignores the re-sparsification that > happens in other commands after the working copy matches the index. > (Such a documentation improvement could come after your series, as I > said, I'm just thinking out loud.) This is a good idea to include in a new "Troubleshooting" section. >> -# 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' ' > > Based on the comments on the next hunk, I also wonder if this hunk > doesn't belong in the previous commit... You are absolutely right, I squashed the wrong commits. Thanks. -Stolee
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index be5e3ac54b8..bb79016d2ca 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p] - [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]] + [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]] [--sparse] [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]] [--] [<pathspec>...] @@ -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. + -i:: --interactive:: Add modified contents in the working tree interactively to diff --git a/builtin/add.c b/builtin/add.c index 09c3fad6321..f8e3930608d 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -30,6 +30,7 @@ static int patch_interactive, add_interactive, edit_interactive; static int take_worktree_changes; static int add_renormalize; static int pathspec_file_nul; +static int include_sparse; static const char *pathspec_from_file; static int legacy_stash_p; /* support for the scripted `git stash` */ @@ -46,7 +47,7 @@ static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only) struct cache_entry *ce = active_cache[i]; int err; - if (ce_skip_worktree(ce)) + if (!include_sparse && ce_skip_worktree(ce)) continue; if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL)) @@ -95,7 +96,7 @@ static void update_callback(struct diff_queue_struct *q, struct diff_filepair *p = q->queue[i]; const char *path = p->one->path; - if (!path_in_sparse_checkout(path, &the_index)) + if (!include_sparse && !path_in_sparse_checkout(path, &the_index)) continue; switch (fix_unmerged_status(p, data)) { @@ -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; @@ -646,7 +649,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (seen[i]) continue; - if (matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) { + if (!include_sparse && + matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) { string_list_append(&only_match_skip_worktree, pathspec.items[i].original); continue; diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 0fdc5c7098c..7d64d9deb22 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -343,11 +343,7 @@ test_expect_success 'commit including unstaged changes' ' test_all_match git status --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_match test_must_fail git add folder1/new && test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && test_sparse_unstaged folder1/new && + test_sparse_match git add --sparse folder1/a && + test_sparse_match git add --sparse folder1/new && - # NEEDSWORK: behavior begins to deviate here. - test_all_match git add . && + test_all_match git add --sparse . && test_all_match git status --porcelain=v2 && test_all_match git commit -m folder1/new && test_all_match git rev-parse HEAD^{tree} && run_on_all ../edit-contents folder1/newer && - test_all_match git add folder1/ && + test_all_match git add --sparse folder1/ && test_all_match git status --porcelain=v2 && test_all_match git commit -m folder1/newer && test_all_match git rev-parse HEAD^{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_match git checkout -b merge-tip merge-left && @@ -555,9 +547,6 @@ test_expect_failure 'merge with conflict outside cone' ' # 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_sparse_match test_must_fail git add folder2 && test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && test_sparse_unstaged folder2/z && @@ -569,7 +558,7 @@ test_expect_failure 'merge with conflict outside cone' ' test_all_match git rev-parse HEAD^{tree} ' -test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' +test_expect_success 'cherry-pick/rebase with conflict outside cone' ' init_repos && for OPERATION in cherry-pick rebase @@ -592,6 +581,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' test_sparse_match test_must_fail git add folder1/a && test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && test_sparse_unstaged folder1/a && + test_all_match git add --sparse folder1/a && test_all_match git status --porcelain=v2 && # 3. Rename the file to another sparse filename and @@ -603,6 +593,7 @@ test_expect_failure 'cherry-pick/rebase with conflict outside cone' ' test_sparse_match test_must_fail git add folder2 && test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err && test_sparse_unstaged folder2/z && + test_all_match git add --sparse folder2 && test_all_match git status --porcelain=v2 && test_all_match git $OPERATION --continue && diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh index 678be1acbf9..0f7e03b5326 100755 --- a/t/t3705-add-sparse-checkout.sh +++ b/t/t3705-add-sparse-checkout.sh @@ -167,7 +167,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' ' git update-index --no-skip-worktree sparse_entry && test_must_fail git add sparse_entry && - test_sparse_entry_unstaged + test_sparse_entry_unstaged && + + # Avoid munging CRLFs to avoid an error message + git -c core.autocrlf=input add --sparse sparse_entry 2>stderr && + test_must_be_empty stderr && + test-tool read-cache --table >actual && + grep "^100644 blob.*sparse_entry\$" actual ' test_expect_success 'add obeys advice.updateSparsePath' ' @@ -178,4 +184,13 @@ test_expect_success 'add obeys advice.updateSparsePath' ' ' +test_expect_success 'add allows sparse entries with --sparse' ' + git sparse-checkout set a && + echo modified >sparse_entry && + test_must_fail git add sparse_entry && + test_sparse_entry_unchanged && + git add --sparse sparse_entry 2>stderr && + test_must_be_empty stderr +' + test_done