On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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;
> }
