Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli <hjemli@gmail.com>]

On Sun, Jan 18, 2009 at 17:13, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Lars Hjemli schrieb:
>> The traversal of submodules is only triggered if the current submodule
>> HEAD commit object is accessible. To this end, read_tree_recursive()
>> will try to insert the submodule odb as an alternate odb but the lack
>> of such an odb is not treated as an error since it is then assumed that
>> the user is not interested in the submodule content. However, if the
>> submodule odb is found it is treated as an error if the HEAD commit
>> object is missing.
>
> Callers of read_tree_recursive() specify a tree to traverse.
> Unconditionally using the HEAD of submodules feels a bit restrictive,
> but I don't use submodules, so I have no idea what I'm actually talking
> about here. :)

For bare repositories (where the submodule repo is added to
objects/info/alternates), following the tree of the linked commit is
the only option. And for non-bare repositories with the submodule
checked out, I think we should honor the users choice of checked out
HEAD in the submodule (especially since we don't have any other way to
specify which submodule commit to follow).


>
>>  int read_tree_recursive(struct tree *tree,
>>                       const char *base, int baselen,
>>                       int stage, const char **match,
>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>>                               return -1;
>>                       continue;
>>               }
>> +             if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>> +                     int retval;
>> +                     char *newbase;
>> +                     struct tree *subtree;
>> +                     unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>> +
>> +                     newbase = xmalloc(baselen + 1 + pathlen);
>> +                     memcpy(newbase, base, baselen);
>> +                     memcpy(newbase + baselen, entry.path, pathlen);
>> +                     newbase[baselen + pathlen] = 0;
>> +                     if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> +                             free(newbase);
>> +                             continue;
>> +                     }
>> +                     newbase[baselen + pathlen] = '/';
>> +                     retval = read_tree_recursive(subtree,
>> +                                                  newbase,
>> +                                                  baselen + pathlen + 1,
>> +                                                  stage, match, fn, context);
>> +                     free(newbase);
>> +                     if (retval)
>> +                             return -1;
>> +                     continue;
>> +             }
>>       }
>>       return 0;
>>  }
>
> You don't need to call get_traverse_gitlinks() in the if statement above
> if you make all read_tree_recursive() callback functions return 0 for
> gitlinks that they don't want to follow and READ_TREE_RECURSIVE for
> those they do.  It's cleaner without the static variable and its
> accessors and more flexible, too: the callbacks might decide to traverse
> only certain submodules.

I like the idea, but it will require thorough review of all
read_tree_recursive() consumers. So now we've got three different
approaches:
* me: global setting
* dscho: parameter to read_tree_recursive()
* you: accept the return value from the callback function

Junio, what would you prefer?

--
larsh