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

[Derrick Stolee <stolee@gmail.com>]

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