Re: [PATCH 1/2] tree.c: allow read_tree_recursive() to traverse gitlink entries

[Lars Hjemli <hjemli@gmail.com>]

On Sun, Jan 25, 2009 at 12:43, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sun, 25 Jan 2009, Lars Hjemli wrote:
>
>> This functionality can be used to allow inter-repository operations, but
>> since the current users of read_tree_recursive() does not yet support
>> such operations, they have been modified where necessary to make sure
>> that they never return READ_TREE_RECURSIVE for gitlink entries (hence no
>> change in behaviour should be introduces by this patch alone).
>
> s/\(introduce\)s/\1d/

Thanks

>
>> diff --git a/archive.c b/archive.c
>> index 9ac455d..e6de039 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -132,7 +132,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>               err = write_entry(args, sha1, path.buf, path.len, mode, NULL, 0);
>>               if (err)
>>                       return err;
>> -             return READ_TREE_RECURSIVE;
>> +             return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
>
> You do not need the parentheses around the conditional:
>
>        $ git grep 'return (.*?' *.c | wc -l
>        14
>        gene099@racer:~/git (rebase-i-p)$ git grep 'return [^(]*?' *.c | wc -l
>        41
>
> Note that the 14 matches include 9 false positives.

Ok, will fix.

>
>> diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
>> index 5b63e6e..fca4631 100644
>> --- a/builtin-ls-tree.c
>> +++ b/builtin-ls-tree.c
>> @@ -68,13 +68,8 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
>>                *
>>                * Something similar to this incomplete example:
>>                *
>> -             if (show_subprojects(base, baselen, pathname)) {
>> -                     struct child_process ls_tree;
>> -
>> -                     ls_tree.dir = base;
>> -                     ls_tree.argv = ls-tree;
>
> I wondered how that could ever have compiled...
>
> Until I inspected the file (which is different in junio/next from what you
> based your patch on; your patch is vs junio/master).

Yes, sorry for not mentioning that.


>
>> @@ -131,6 +131,34 @@ int read_tree_recursive(struct tree *tree,
>>                       if (retval)
>>                               return -1;
>>                       continue;
>> +             } else if (S_ISGITLINK(entry.mode)) {
>> +                     int retval;
>> +                     struct strbuf path;
>
> s/;/ = STRBUF_INIT;/

I skipped the STRBUF_INIT since I used strbuf_init() below, but...


>
>> +                     unsigned int entrylen;
>> +                     struct commit *commit;
>> +
>> +                     entrylen = tree_entry_len(entry.path, entry.sha1);
>> +                     strbuf_init(&path, baselen + entrylen + 1);
>> +                     strbuf_add(&path, base, baselen);
>> +                     strbuf_add(&path, entry.path, entrylen);
>> +                     strbuf_addch(&path, '/');
>
> Why not
>                        strbuf_addf(&path, "%.*s%.*s/", baselen, base,
>                                entrylen, entry.path);

...with this cute fix the STRBUF_INIT is required. Will fix.

Thanks for the review.

--
larsh