Re: [PATCH v5 08/14] unpack-trees: unpack sparse directory entries

> On Mon, Jun 7, 2021 at 5:34 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> During unpack_callback(), index entries are compared against tree
>> entries. These are matched according to names and types. One goal is to
>> decide if we should recurse into subtrees or simply operate on one index
>> entry.
>>
>> In the case of a sparse-directory entry, we do not want to recurse into
>> that subtree and instead simply compare the trees. In some cases, we
>> might want to perform a merge operation on the entry, such as during
>> 'git checkout <commit>' which wants to replace a sparse tree entry with
>> the tree for that path at the target commit. We extend the logic within
>> unpack_nondirectories() to create a sparse-directory entry in this case,
>> and then that is sent to call_unpack_fn().
> 
> Does this presume that all callbacks are prepared to accept a sparse
> directory entry?  Or do we have an external flag that ensures we do
> not reach this code path when using callbacks that aren't prepared to
> handle it properly?
> 
> I hope that the answer is the latter, and that the ensure_full_index()
> calls are what prevents the code from reaching this point if a
> callback would be used that couldn't handle a sparse directory entry.

> I'd be particularly concerned that merge-recursive would call this
> code with unpack_opts.fn = threeway_merge.  threeway_merge is kind of
> interesting in that it might just happen to not die when passed a
> sparse directory entry, but would pass along data that'd just break
> stuff downstream in various subtle ways.  For example, if there were
> conflicts in the sparse directory entries because both had been
> modified, the merge should recurse and resolve individual paths
> underneath, which the merge-recursive code would not be prepared to do
> since unpack_trees() has already returned.  Also, even if there wasn't
> a "conflict" because only one side modified, blindly doing a trivial
> directory resolution will break rename detection.  I mention
> merge-recursive not because it's worth fixing (well, it was and the
> fix is called merge-ort) but because I'm most familiar with it.  The
> other callbacks _might_ have similar problems, though its possible
> that it's safe for one- and two- way merging and just fails once you
> get to three-way.

>> +       if (is_sparse_directory) {
>> +               ce->name[len] = '/';
>> +               ce->name[len + 1] = 0;
> 
> Should this be '\0', for clarity?

>> @@ -1064,16 +1073,24 @@ static int unpack_nondirectories(int n, unsigned long mask,
>>                                  unsigned long dirmask,
>>                                  struct cache_entry **src,
>>                                  const struct name_entry *names,
>> -                                const struct traverse_info *info)
>> +                                const struct traverse_info *info,
>> +                                int sparse_directory)
>>  {
>>         int i;
>>         struct unpack_trees_options *o = info->data;
>>         unsigned long conflicts = info->df_conflicts | dirmask;
>>
>> -       /* Do we have *only* directories? Nothing to do */
> 
> You've removed the comment, but not the code.  So it still returns
> immediately if there are only directories...right?  Am I missing
> something?  Is this code still correct?  Or is the comment just
> misleading now that src[0] can be a directory?

>>         if (mask == dirmask && !src[0])
>>                 return 0;
>>
>> +       /* no-op if our cache entry doesn't match the expectations. */
>> +       if (sparse_directory) {
>> +               if (src[0] && !S_ISSPARSEDIR(src[0]->ce_mode))
>> +                       BUG("expected sparse directory entry");
>> +       } else if (src[0] && S_ISSPARSEDIR(src[0]->ce_mode)) {
>> +               return 0;
>> +       }
> 
> This code reads like "If sparse_directory is false, but the cache
> entry is a sparse directory, we'll just keep it as-is and ignore
> changed or conflicting directories or files from the names name_entry.
> However, I think this has to be coupled with knowledge about changes
> to unpack_callback() you made, where you introduce an extra call to
> unpack_nondirectories() for the sparse directory case, and in the
> second one you would do useful work.  So "no-op" is kind of
> misleading, it's more deferral until the later unpack_nondirectories()
> call.
> 
> Or, at least so I think after trying to read over this patch.  Am I
> understanding this right?

>> +
>> +       /*
>> +        * Compare ce->name to info->name + '/' + p->path + '/'
>> +        * if info->name is non-empty. Compare ce->name to
>> +        * p-.path + '/' otherwise.
> 
> p->path, not p-.path

> Also, you state in both cases that you are comparing against a
> trailing '/', but...
> 
>> +        */
>> +       if (info->namelen) {
>> +               if (ce->ce_namelen == info->namelen + p->pathlen + 2 &&
>> +                   ce->name[info->namelen] == '/' &&
>> +                   !strncmp(ce->name, info->name, info->namelen) &&
>> +                   !strncmp(ce->name + info->namelen + 1, p->path, p->pathlen))
> 
> You only checked for one of the two '/' characters here.

>> +                       return ce;
>> +       } else if (ce->ce_namelen == p->pathlen + 1 &&
>> +                  !strncmp(ce->name, p->path, p->pathlen))
> 
> Here you didn't check for the final '/'.  Is that intentional because
> of the S_ISSPARSEDIR() check above?  If so, should the comment above
> this block be corrected?

>> +               return ce;
>> +       return NULL;
>>  }
>>
>>  static void debug_path(struct traverse_info *info)
>> @@ -1251,6 +1301,32 @@ static void debug_unpack_callback(int n,
>>                 debug_name_entry(i, names + i);
>>  }
>>
>> +/*
>> + * Returns true if and only if the given cache_entry is a
>> + * sparse-directory entry that matches the given name_entry
>> + * from the tree walk at the given traverse_info.
>> + */
>> +static int is_sparse_directory_entry(struct cache_entry *ce, struct name_entry *name, struct traverse_info *info)
>> +{
>> +       size_t expected_len, name_start;
>> +
>> +       if (!ce || !name || !S_ISSPARSEDIR(ce->ce_mode))
>> +               return 0;
>> +
>> +       if (info->namelen)
>> +               name_start = info->namelen + 1;
>> +       else
>> +               name_start = 0;
>> +       expected_len = name->pathlen + 1 + name_start;
>> +
>> +       if (ce->ce_namelen != expected_len ||
>> +           strncmp(ce->name, info->name, info->namelen) ||
>> +           strncmp(ce->name + name_start, name->path, name->pathlen))
>> +               return 0;
> 
> What about the intervening '/' character?  Could we get a false hit
> between "foo/bar/" and "foo.bar/"?