[PATCH 0/2] Add submodule-support to git archive

[PATCH 0/2] Add submodule-support to git archive

[Lars Hjemli]

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

To make the `--submodules` option more userfriendly, any submodule repository
discovered during traversal will be registered as an alternate odb (this
will typically be required to make the inter-repository traversal succeed).

Finally, the option `--submodules=group:<name>` is not yet implemented. I
wanted to get these first two patches published early since they define the
semantics of the --submodules option. Adding a 'group' selector on top is
mostly a question of pulling information out of .gitmodules and .git/config,
i.e. not very exciting (but it will be done ;-)

Lars Hjemli (2):
  tree.c: allow read_tree_recursive() to traverse gitlink entries
  archive.c: add support for --submodules[=(all|checkedout)]

 Documentation/git-archive.txt |    5 ++
 archive.c                     |   81 +++++++++++++++++++++++++-
 archive.h                     |    4 +
 builtin-ls-tree.c             |    9 +--
 cache.h                       |    1 +
 merge-recursive.c             |    2 +-
 sha1_file.c                   |   11 +++-
 t/t5001-archive-submodules.sh |  129 +++++++++++++++++++++++++++++++++++++++++
 tree.c                        |   28 +++++++++
 9 files changed, 259 insertions(+), 11 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

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

[Lars Hjemli]

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

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 archive.c         |    2 +-
 builtin-ls-tree.c |    9 ++-------
 merge-recursive.c |    2 +-
 tree.c            |   28 ++++++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 9 deletions(-)

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);
 	}
 
 	buffer = sha1_file_to_archive(path_without_prefix, sha1, mode,
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;
-			start_command(&ls_tree);
-		}
+		if (show_subprojects(base, baselen, pathname))
+			retval = READ_TREE_RECURSIVE;
 		 *
 		 */
 		type = commit_type;
diff --git a/merge-recursive.c b/merge-recursive.c
index b97026b..ee853b9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -237,7 +237,7 @@ static int save_files_dirs(const unsigned char *sha1,
 		string_list_insert(newpath, &o->current_file_set);
 	free(newpath);
 
-	return READ_TREE_RECURSIVE;
+	return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 }
 
 static int get_files_dirs(struct merge_options *o, struct tree *tree)
diff --git a/tree.c b/tree.c
index 03e782a..dfe4d5f 100644
--- a/tree.c
+++ b/tree.c
@@ -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;
 		}
 	}
 	return 0;
-- 
1.6.1.150.g5e733b

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

[Lars Hjemli]

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

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/git-archive.txt |    5 ++
 archive.c                     |   81 +++++++++++++++++++++++++-
 archive.h                     |    4 +
 cache.h                       |    1 +
 sha1_file.c                   |   11 +++-
 t/t5001-archive-submodules.sh |  129 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 228 insertions(+), 3 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 41cbf9c..6068302 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -47,6 +47,11 @@ OPTIONS
 --prefix=<prefix>/::
 	Prepend <prefix>/ to each filename in the archive.
 
+--submodules[=<spec>]::
+	Include the content of submodules in the archive. The specification
+	of which submodules to include can be either 'checkedout' (default)
+	or 'all'.
+
 <extra>::
 	This can be any options that the archiver backend understand.
 	See next section.
diff --git a/archive.c b/archive.c
index e6de039..bb0c5c8 100644
--- a/archive.c
+++ b/archive.c
@@ -4,6 +4,7 @@
 #include "attr.h"
 #include "archive.h"
 #include "parse-options.h"
+#include "refs.h"
 
 static char const * const archive_usage[] = {
 	"git archive [options] <tree-ish> [path...]",
@@ -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;
+}
+
+static int check_gitlink(struct archiver_args *args, const unsigned char *sha1,
+			 const char *path)
+{
+	switch (args->submodules) {
+	case 0:
+		return 0;
+
+	case SUBMODULES_ALL:
+		/* When all submodules are requested, we try to add any
+		 * checked out submodules as alternate odbs. But we don't
+		 * really care whether any particular submodule is checked
+		 * out or not, we are going to try to traverse it anyways.
+		 */
+		include_repository(mkpath("%s.git", path));
+		return READ_TREE_RECURSIVE;
+
+	case SUBMODULES_CHECKEDOUT:
+		/* If a repo is checked out at the gitlink path, we want to
+		 * traverse into the submodule. But we ignore the current
+		 * HEAD of the checked out submodule and always uses the SHA1
+		 * recorded in the gitlink entry since we want the content
+		 * of the archive to match the content of the <tree-ish>
+		 * specified on the command line.
+		 */
+		if (!include_repository(mkpath("%s.git", path)))
+			return READ_TREE_RECURSIVE;
+		else
+			return 0;
+
+	default:
+		die("archive.c: invalid value for args->submodules: %d",
+		    args->submodules);
+	}
+}
+
 struct archiver_context {
 	struct archiver_args *args;
 	write_archive_entry_fn_t write_entry;
@@ -132,7 +197,8 @@ 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 (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
+		return (S_ISDIR(mode) ? READ_TREE_RECURSIVE :
+			check_gitlink(args, sha1, path.buf));
 	}
 
 	buffer = sha1_file_to_archive(path_without_prefix, sha1, mode,
@@ -253,6 +319,7 @@ static int parse_archive_args(int argc, const char **argv,
 	const char *base = NULL;
 	const char *remote = NULL;
 	const char *exec = NULL;
+	const char *submodules = NULL;
 	int compression_level = -1;
 	int verbose = 0;
 	int i;
@@ -262,6 +329,9 @@ static int parse_archive_args(int argc, const char **argv,
 		OPT_STRING(0, "format", &format, "fmt", "archive format"),
 		OPT_STRING(0, "prefix", &base, "prefix",
 			"prepend prefix to each pathname in the archive"),
+		{OPTION_STRING, 0, "submodules", &submodules, "kind",
+			"include submodule content in the archive",
+			PARSE_OPT_OPTARG, NULL, (intptr_t)"checkedout"},
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -316,6 +386,15 @@ static int parse_archive_args(int argc, const char **argv,
 					format, compression_level);
 		}
 	}
+
+	if (!submodules)
+		args->submodules = 0;
+	else if (!strcmp(submodules, "checkedout"))
+		args->submodules = SUBMODULES_CHECKEDOUT;
+	else if (!strcmp(submodules, "all"))
+		args->submodules = SUBMODULES_ALL;
+	else
+		die("Invalid submodule kind: %s", submodules);
 	args->verbose = verbose;
 	args->base = base;
 	args->baselen = strlen(base);
diff --git a/archive.h b/archive.h
index 0b15b35..2b078b6 100644
--- a/archive.h
+++ b/archive.h
@@ -11,8 +11,12 @@ struct archiver_args {
 	const char **pathspec;
 	unsigned int verbose : 1;
 	int compression_level;
+	int submodules;
 };
 
+#define SUBMODULES_CHECKEDOUT 1
+#define SUBMODULES_ALL 2
+
 typedef int (*write_archive_fn_t)(struct archiver_args *);
 
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args, const unsigned char *sha1, const char *path, size_t pathlen, unsigned int mode, void *buffer, unsigned long size);
diff --git a/cache.h b/cache.h
index 8d965b8..ea53e4b 100644
--- a/cache.h
+++ b/cache.h
@@ -728,6 +728,7 @@ extern struct alternate_object_database {
 	char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
+extern int add_alt_odb(const char *path);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index 360f7e5..53d8db7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -285,8 +285,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 
 	/* Detect cases where alternate disappeared */
 	if (!is_directory(ent->base)) {
-		error("object directory %s does not exist; "
-		      "check .git/objects/info/alternates.",
+		error("Alternate object directory %s does not exist",
 		      ent->base);
 		free(ent);
 		return -1;
@@ -2573,3 +2572,11 @@ int read_pack_header(int fd, struct pack_header *header)
 		return PH_ERROR_PROTOCOL;
 	return 0;
 }
+
+int add_alt_odb(const char *path)
+{
+	int err = link_alt_odb_entry(path, strlen(path), NULL, 0);
+	if (!err)
+		prepare_packed_git_one((char *)path, 0);
+	return err;
+}
diff --git a/t/t5001-archive-submodules.sh b/t/t5001-archive-submodules.sh
new file mode 100755
index 0000000..14383b3
--- /dev/null
+++ b/t/t5001-archive-submodules.sh
@@ -0,0 +1,129 @@
+#!/bin/sh
+
+test_description='git archive can include submodule content'
+
+. ./test-lib.sh
+
+add_file()
+{
+	git add $1 &&
+	git commit -m "added $1"
+}
+
+add_submodule()
+{
+	mkdir $1 && (
+		cd $1 &&
+		git init &&
+		echo "File $2" >$2 &&
+		add_file $2
+	) &&
+	add_file $1
+}
+
+test_expect_success 'by default, submodules are not included' '
+	echo "File 1" >1 &&
+	add_file 1 &&
+	add_submodule 2 3 &&
+	add_submodule 4 5 &&
+	cat <<EOF >expected &&
+1
+2/
+4/
+EOF
+	git archive HEAD >normal.tar &&
+	tar -tf normal.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with --submodules, checked out submodules are  included' '
+	cat <<EOF >expected &&
+1
+2/
+2/3
+4/
+4/5
+EOF
+	git archive --submodules HEAD >full.tar &&
+	tar -tf full.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with --submodules=all, all submodules are included' '
+	git archive --submodules=all HEAD >all.tar &&
+	tar -tf all.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'submodules in submodules are supported' '
+	(cd 4 && add_submodule 6 7) &&
+	add_file 4 &&
+	cat <<EOF >expected &&
+1
+2/
+2/3
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	git archive --submodules HEAD >recursive.tar &&
+	tar -tf recursive.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'packed submodules are supported' '
+	msg=$(cd 2 && git repack -ad && git count-objects) &&
+	test "$msg" = "0 objects, 0 kilobytes" &&
+	git archive --submodules HEAD >packed.tar &&
+	tar -tf packed.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'missing submodule packs triggers an error' '
+	mv 2/.git/objects/pack .git/packdir2 &&
+	test_must_fail git archive --submodules HEAD
+'
+
+test_expect_success '--submodules skips non-checked out submodules' '
+	cat <<EOF >expected &&
+1
+2/
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	rm -rf 2/.git &&
+	git archive --submodules HEAD >partial.tar &&
+	tar -tf partial.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--submodules=all fails if gitlinked objects are missing' '
+	test_must_fail git archive --submodules=all HEAD
+'
+
+test_expect_success \
+	'--submodules=all does not require submodules to be checked out' '
+	cat <<EOF >expected &&
+1
+2/
+2/3
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	mv .git/packdir2/* .git/objects/pack/ &&
+	git archive --submodules=all HEAD >all2.tar &&
+	tar -tf all2.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'missing objects in a submodule triggers an error' '
+	find 4/.git/objects -type f | xargs rm &&
+	test_must_fail git archive --submodules HEAD
+'
+
+test_done
-- 
1.6.1.150.g5e733b

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

[Johannes Schindelin]

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?

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

Ciao,
Dscho

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

[Lars Hjemli]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

[Lars Hjemli]

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

Re: [PATCH 0/2] Add submodule-support to git archive

[Nanako Shiraishi]

Quoting Lars Hjemli <hjemli@gmail.com>:

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

I wanted to try this because I use submodules in a repository at my work, but your patches conflicted a lot with your previous series of patches that are already in the next branch of Junio's. 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?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH 0/2] Add submodule-support to git archive

[Lars Hjemli]

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

--
larsh

Re: [PATCH 0/2] Add submodule-support to git archive

[Junio C Hamano]

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.

Could we switch to incremental refinements once a series hits next, pretty
please?

Re: [PATCH 0/2] Add submodule-support to git archive

[Lars Hjemli]

On Sun, Jan 25, 2009 at 21:35, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Sorry about that.


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

--
larsh

Re: [PATCH 0/2] Add submodule-support to git archive

[Johannes Schindelin]

Hi,

On Mon, 26 Jan 2009, Lars Hjemli wrote:

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

I'd appreciate that.

Since 'next' will be rewound eventually, that first iteration and the 
revert will disappear at some stage.

Ciao,
Dscho

Re: [PATCH 0/2] Add submodule-support to git archive

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

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

Ok, then I'll simply revert and then queue the new ones on top of it.

Thanks.