Re: [PATCH 2/2] archive.c: add support for --submodules[=(all|checkedout)]

[Lars Hjemli <hjemli@gmail.com>]

On Sun, Jan 25, 2009 at 12:57, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 25 Jan 2009, Lars Hjemli wrote:
>
>> The --submodules option uses the enhanced read_tree_recursive() to
>> enable inclusion of submodules in the generated archive.
>>
>> When invoked with `--submodules=all` all gitlink entries will be
>> traversed, and when invoked with --submodules=checkedout (the default
>> option) only gitlink entries with a git repo (i.e. checked out sub-
>> modules) will be traversed.
>
>                               In bare repositories, this means: none.
>
> My reasoning for "*" instead of "all" and "" instead for "checkedout" was
> that you could allow "<name1>,<name2>" at some stage, where <name> would
> first be interpreted as a submodule group, and if that fails, as submodule
> name.
>
> Thinking about that more, "" seems illogical, that should rather mean
> "none", i.e. the same as --no-submodules.  The "checkedout" could be "."
> then, perhaps?  As in "what we have checked out in ./, the current
> directory"?

Yes, I think that makes sense, i.e. '--submodules' will include _all_
submodules (making the option behave identically for bare and non-bare
repositories), '--submodules=.' will include checked out submodules
(making the option a no-op in bare repos, which also makes sense) and
'--submodules=<name>[,<name>...]' will include the named submodules,
where "named" could mean groupname, submodule name or submodule path,
in that order.

But then we probably also want some (optional) syntax to specify the
kind of name, e.g. '--submodules=g:foo,n:bar,p:lib/baz' for group foo,
name bar and path lib/baz. Agree?

>
>> When a gitlink has been selected for traversal, it is required that all
>> objects necessary to perform this traversal are available in either the
>> primary odb or through an alternate odb. To this end, git archive will
>> insert the object database of the selected gitlink (when checked out)
>> as an alternate odb, using the new function add_alt_odb().
>
>> And since alternates now can be added after parsing of
>> objects/info/alternates, the error message in link_alt_odb_entry() has
>> been updated to not mention this file.
>
> Could you split that part into its own patch again, please?

Sure.


>
>> @@ -91,6 +92,70 @@ static void setup_archive_check(struct git_attr_check *check)
>>       check[1].attr = attr_export_subst;
>>  }
>>
>> +static int include_repository(const char *path)
>> +{
>> +     struct stat st;
>> +     const char *tmp;
>> +
>> +     /* Return early if the path does not exist since it is OK to not
>> +      * checkout submodules.
>> +      */
>> +     if (stat(path, &st) && errno == ENOENT)
>> +             return 1;
>> +
>> +     tmp = read_gitfile_gently(path);
>
> This will leak memory, no?

I don't think so: read_gitfile_gently() returns a value obtained by
calling make_absolute_path() which returns a static buffer. Also, the
path argument to include_repository() is obtained by calling mkpath()
which returns another static buffer so I don't see any malloc()'s
which should be free()'d. Is my code-reading flawed?

--
larsh