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

On 8/27/2021 5:06 PM, Matheus Tavares Bernardino wrote:

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

Thanks for adding your review. I'm sorry I'm so late getting back to it.

quoted

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 sprase-checkout cone.

s/sprase/sparse/

Thanks.

quoted

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.

quoted

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