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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 25 Jan 2009, Lars Hjemli wrote:

> On Sun, Jan 25, 2009 at 12:57, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > 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.

Well, I can live with the default of all submodules, even if I think that 
"git-submodule.sh" uses the checked out submodules by default.

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

IMO that is overkill.  Anybody naming a submodule group identically to a 
submodule deserves what she gets, anyway.


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

No, your code reading is good.  And you spared me having to read the code 
myself ;-)  Now, maybe a code comment is in order, to spare others, too?

Ciao,
Dscho