Re: [PATCH 05/13] add: fail when adding an untracked sparse file

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Aug 27, 2021, 21:06 UTC
Message-ID: <CAHd-oW7D1jikE5ByS36AjACfSJoZeekLCKzX2MRRfayKwKv=qQ@mail.gmail.com>
In-Reply-To: <e80fcfa932cca394c5c8b349cafadb0754a594dd.1629842085.git.gitgitgadget@gmail.com>

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

>
> 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/

Show 12 quoted lines

> 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()`?

Show 15 quoted lines

> 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?

Show 21 quoted lines

>         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?

Show 6 quoted lines

> +               exit_status = 1;
> +       }
> +       string_list_clear(&only_match_skip_worktree, 0);
> +
>         return exit_status;
>  }