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

[Derrick Stolee <stolee@gmail.com>]

On 6/8/2021 11:48 PM, Elijah Newren wrote:
> 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.

To the best of my knowledge, callbacks that are not protected have
ensure_full_index() protecting them. At minimum, the repository setting
command_requires_full_index is enabled by default, causing a sparse
index to be expanded to a full one immediately upon parsing (and also
after writing) to protect cases that might be missing. That is, until
we can create tests for each command before disabling it for that
command.

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

I also believe that threeway_merge might be difficult to integrate
and that its use should be protected with ensure_full_index() even if
'git merge' in general does not do it by default. We can cross that
bridge when we get to it. Merge, rebase, and cherry-pick are next on
my list of "commands to integrate with sparse-index" but I haven't
done the work yet to make them work.
>> +       if (is_sparse_directory) {
>> +               ce->name[len] = '/';
>> +               ce->name[len + 1] = 0;
> 
> Should this be '\0', for clarity?

Sure.

>> @@ -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?
Yes, the comment is misleading now that we can call this method
with sparse-directory entries. The method name is also a bit
misleading: this should perhaps be renamed to unpack_single_entry()
or something like that. That will signal that we are not recursing
with traverse_trees_recursive() as we do in the other case.
 
>>         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?

I think they are both correct: we defer until later by doing a no-op
right now. But using "defer" is more informative of the context of
this call.

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

Thanks!
 
> 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.

The first '/' check is to verify that we match "{info->name}/{p->name}/"
and not "{info->name}.{p->name}/" ('.' means "any character").

>  Are you
> omitting the check for the final '/' do to the S_ISSPARSEDIR() check
> above?

Since we know at this point that ce is a sparse directory entry, the
final character _must_ be a trailing slash. There is not a trailing
slash in the input p->path.

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

Yes, will do.

>> +               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/"?

Here, you are right that I missed this check. I will add it.

> Also, do we have to worry about the trailing '/'?

No, the index would be malformed without it. Since this code
is so similar to the other check (just the negation of it)
I will add a clearly-commented helper method.

Thanks,
-Stolee