patch, 2 partstree.c: allow read_tree_recursive() to traverse gitlink entries

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

> 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().

> @@ -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);

> +	if (tmp) {
> +		path = tmp;
> +		if (stat(path, &st))
> +			die("Unable to stat submodule gitdir %s: %s (%d)",
> +			    path, strerror(errno), errno);
> +	}
> +
> +	if (!S_ISDIR(st.st_mode))
> +		die("Submodule gitdir %s is not a directory", path);
> +
> +	if (add_alt_odb(mkpath("%s/objects", path)))
> +		die("submodule odb %s could not be added as an alternate",
> +		    path);
> +
> +	return 0;
> +}

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

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

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

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

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

> 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).

> 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);

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

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

> +			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, '/');

> +
> +			commit = lookup_commit(entry.sha1);
> +			if (!commit)
> +				die("Commit %s in submodule path %s not found",
> +				    sha1_to_hex(entry.sha1), path.buf);
> +
> +			if (parse_commit(commit))
> +				die("Invalid commit %s in submodule path %s",
> +				    sha1_to_hex(entry.sha1), path.buf);
> +
> +			retval = read_tree_recursive(commit->tree,
> +						     path.buf, path.len,
> +						     stage, match, fn, context);
> +			strbuf_release(&path);
> +			if (retval)
> +				return -1;
> +			continue;

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

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

>
>> 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).

>
>> @@ -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;/

>
>> +                     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);

> This is a cleaned up version of my previous patches which allows git archive
> to include submodule content in the archive output.
>
> The main difference between this series and the previous ones is that the
> behaviour of `git archive --submodules` are now predictable; the content
> included from submodules is defined by the gitlink entries found when
> traversing the <tree-ish> specified on the command line, and the set of
> submodules to include are defined by specifying either `--submodules=all` or
> `--submodules=checkedout` (which is the default mode of operation, i.e. what
> you get by only specifying `--submodules`).

> On Sun, Jan 25, 2009 at 05:53, Nanako Shiraishi <nanako3@lavabit.com> wrote:
>> What would I do to try this new series? Fork a branch from Junio's master branch,
>> apply your new patches, and merge the result to Junio's next?
>
> Yes, that sounds right (btw: the series is buildt on top of 5dc1308562
> (Merge branch 'js/patience-diff') and can be pulled from
> git://hjemli.net/pub/git/git lh/traverse-gitlinks).
>
> But before merging with 'next', you'll need to `git revert -m 1 bdf31cbc00`.

> Lars Hjemli <hjemli@gmail.com> writes:
>
>> On Sun, Jan 25, 2009 at 05:53, Nanako Shiraishi <nanako3@lavabit.com> wrote:
>>> What would I do to try this new series? Fork a branch from Junio's master branch,
>>> apply your new patches, and merge the result to Junio's next?
>>
>> Yes, that sounds right (btw: the series is buildt on top of 5dc1308562
>> (Merge branch 'js/patience-diff') and can be pulled from
>> git://hjemli.net/pub/git/git lh/traverse-gitlinks).
>>
>> But before merging with 'next', you'll need to `git revert -m 1 bdf31cbc00`.
>
> Yuck, that is too much to ask for regular testers and users.

> On Sun, Jan 25, 2009 at 21:35, Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Could we switch to incremental refinements once a series hits next, 
> > pretty please?
> 
> The problem in this particular case is that the design has changed so 
> much since the first iteration that we're not really talking about 
> incremental refinements but rather a different approach to the same 
> problem.
> 
> If you want me to build on top of the series in next anyways, would it
> be acceptable if the first patch on top of ee306d2d59 reverts the
> previous attempt? I think the rest of the series will be easier to
> review that way.