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

[Lars Hjemli <hjemli@gmail.com>]

On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> diff --git a/environment.c b/environment.c
>> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)
>>       setup_git_env();
>>       return 0;
>>  }
>> +
>> +int get_traverse_gitlinks()
>> +{
>> +     return traverse_gitlinks;
>> +}
>> +
>> +void set_traverse_gitlinks(int traverse)
>> +{
>> +     traverse_gitlinks = traverse;
>> +}
>
> If you have full accessors anyway, it is much easier and cleaner to make
> this a global variable to begin with.
>
> However, environment.c is reserved for things that come from the config
> and can be overridden by the user.  That is certainly not the case for
> traverse_gitlinks.
>
> But let's think about it again: should traverse_gitlinks be a global
> varible at all?  I think not.  It should be a per-call decision.

Yes, it might be a cleaner solution to add an extra parameter to
read_tree_recursive(), and since it currently has only 7 callsites the
patch wouldn't be too big either. Junio, what do you think?


>> +/* Try to add the objectdb of a submodule */
>> +int add_gitlink_odb(char *relpath)
>
> This wants to be static.

Thanks

>
>> +{
>> +     const char *odbpath;
>> +     struct stat st;
>> +
>> +     odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
>> +     if (!odbpath)
>> +             odbpath = mkpath("%s/.git/objects", relpath);
>> +
>> +     if (stat(odbpath, &st))
>> +             return 1;
>> +
>> +     return add_alt_odb(odbpath);
>> +}
>> +
>> +/* Check if we should recurse into the specified submodule */
>> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,
>
> This, too.

Thanks again ;-)

>
>> +                  struct tree **subtree)
>> +{
>> +     unsigned char sha1[20];
>> +     int linked_odb = 0;
>> +     struct commit *commit;
>> +     void *buffer;
>> +     enum object_type type;
>> +     unsigned long size;
>> +
>> +     hashcpy(sha1, commit_sha1);
>> +     if (!add_gitlink_odb(path)) {
>> +             linked_odb = 1;
>> +             if (resolve_gitlink_ref(path, "HEAD", sha1))
>> +                     die("Unable to lookup HEAD in %s", path);
>> +     }
>
> Why would you want to continue if add_gitlink_odb() did not find a checked
> out submodule?
>
> Seems you want to fall back to look in the superproject's object database.
> But I think that is wrong, as I have a superproject with many platform
> dependent submodules, only one of which is checked out, and for
> convenience, the submodules all live in the superproject's repository.

Actually, I want this to work for bare repositories by specifying the
submodule odbs in the alternates file. So if the current submodule odb
wasn't found my plan was to check if the commit object was accessible
anyways but don't die() if it wasn't.


>> +     commit = lookup_commit(sha1);
>> +     if (!commit)
>> +             die("traverse_gitlink(): internal error");
>
> s/internal error/could not access commit '%s' of submodule '%s'",
>                        sha1_to_hex(sha1), path);/

Ok (I belive this codepath is virtually impossible to hit, hence the
"internal error", but I could of course be mistaken).


>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>>                               return -1;
>>                       continue;
>>               }
>> +             if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>
> Like I said, traverse_gitlinks should be a flag to read_tree_recursive.
> So preferably, you should add a parameter 'flags' and make that option an
> enum.
>
>> +                     int retval;
>> +                     char *newbase;
>> +                     struct tree *subtree;
>> +                     unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>
> Nit: Long line.

Will fix

>
>> +
>> +                     newbase = xmalloc(baselen + 1 + pathlen);
>> +                     memcpy(newbase, base, baselen);
>> +                     memcpy(newbase + baselen, entry.path, pathlen);
>> +                     newbase[baselen + pathlen] = 0;
>
> We have strbufs for that.
>
>> +                     if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> +                             free(newbase);
>> +                             continue;
>> +                     }
>> +                     newbase[baselen + pathlen] = '/';
>
> ... to avoid this off-by-one.

Actually, I don't think this is off-by-one, since (baselen + pathlen +
1) is passed along to read_tree_recursive() as the new baselen. But
using strbufs might be cleaner anyways.

Thanks for the review.

--
larsh