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

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

Hi,

On Sun, 25 Jan 2009, Lars Hjemli wrote:

> When the callback function invoked from read_tree_recursive() returns
> the value `READ_TREE_RECURSIVE` for a gitlink entry, the traversal will
> now continue into the tree connected to the gitlinked commit.

\n

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

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

I'd also place a comment above read_tree_recursive() stating that this 
function tries to traverse into submodules when READ_TREE_RECURSIVE is 
returned for submodule entries, but no attempt is made at including 
alternate object directories.  (And it must be that way: think bare 
repositories -- they cannot just try to include a subdirectory's 
.git/objects/..)

Ciao,
Dscho