[RFC/PATCH v3 0/3] Add support for `git archive --submodules`

This series teaches read_tree_recursive() how to traverse gitlink entries
when explicitly instructed to do so (by the return value from the provided
callback function) and then uses this functionallity in git-archive to
implement a basic --submodules option (as suggested by René in
http://thread.gmane.org/gmane.comp.version-control.git/106167/focus=106235).

The commit message of the third patch has some suggestions on how the new
feature may be extended to support more use cases - hopefully this will
cover the issues mentioned by Johannes and Junio in the same thread.

Lars Hjemli (3):
  tree.c: teach read_tree_recursive how to traverse gitlink entries
  sha1_file: prepare for adding alternates on demand
  archive.c: add basic support for submodules

 Documentation/git-archive.txt |    3 +
 archive.c                     |   53 ++++++++++++++++++-
 archive.h                     |    1 +
 builtin-ls-tree.c             |    9 +---
 cache.h                       |    1 +
 merge-recursive.c             |    2 +-
 sha1_file.c                   |   40 +++++++++-----
 t/t5001-archive-submodules.sh |  121 +++++++++++++++++++++++++++++++++++++++++
 tree.c                        |   28 ++++++++++
 9 files changed, 236 insertions(+), 22 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

The new function add_alt_odb() can be used to add alternate object
databases dynamically (i.e. after parsing of objects/info/alternates).
It will be used by git-archive to implement inclusion of submodules
by adding submodule object databases during tree traversal.

To make the function usable from call-sites which doesn't require the
add_alt_odb() to succeed, it takes a 'quiet' parameter which is passed
on to the underlying alt-odb-related functions.

Signed-off-by: Lars Hjemli <redacted>
---
 cache.h     |    1 +
 sha1_file.c |   40 +++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 8e1af26..ccfad5f 100644
--- a/cache.h
+++ b/cache.h

@@ -724,6 +724,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(char *path, int quiet);
 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 f08493f..8b5540d 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -235,7 +235,7 @@ char *sha1_pack_index_name(const unsigned char *sha1)
 struct alternate_object_database *alt_odb_list;
 static struct alternate_object_database **alt_odb_tail;
 
-static void read_info_alternates(const char * alternates, int depth);
+static void read_info_alternates(const char * alternates, int depth, int quiet);
 
 /*
  * Prepare alternate object database registry.

@@ -252,7 +252,8 @@ static void read_info_alternates(const char * alternates, int depth);
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)
+static int link_alt_odb_entry(const char * entry, int len,
+			      const char * relative_base, int depth, int quiet)
 {
 	const char *objdir = get_object_directory();
 	struct alternate_object_database *ent;

@@ -285,9 +286,10 @@ 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.",
-		      ent->base);
+		if (!quiet)
+			error("object directory %s does not exist; "
+			      "check .git/objects/info/alternates.",
+			      ent->base);
 		free(ent);
 		return -1;
 	}

@@ -312,7 +314,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 	ent->next = NULL;
 
 	/* recursively add alternates */
-	read_info_alternates(ent->base, depth + 1);
+	read_info_alternates(ent->base, depth + 1, quiet);
 
 	ent->base[pfxlen] = '/';
 

@@ -320,7 +322,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 }
 
 static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
-				 const char *relative_base, int depth)
+				 const char *relative_base, int depth,
+				 int quiet)
 {
 	const char *cp, *last;
 

@@ -343,11 +346,12 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 			cp++;
 		if (last != cp) {
 			if (!is_absolute_path(last) && depth) {
+				if (!quiet)
 				error("%s: ignoring relative alternate object store %s",
 						relative_base, last);
 			} else {
 				link_alt_odb_entry(last, cp - last,
-						relative_base, depth);
+						relative_base, depth, quiet);
 			}
 		}
 		while (cp < ep && *cp == sep)

@@ -356,7 +360,8 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 	}
 }
 
-static void read_info_alternates(const char * relative_base, int depth)
+static void read_info_alternates(const char * relative_base, int depth,
+				 int quiet)
 {
 	char *map;
 	size_t mapsz;

@@ -380,7 +385,8 @@ static void read_info_alternates(const char * relative_base, int depth)
 	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
 
-	link_alt_odb_entries(map, map + mapsz, '\n', relative_base, depth);
+	link_alt_odb_entries(map, map + mapsz, '\n', relative_base, depth,
+			     quiet);
 
 	munmap(map, mapsz);
 }

@@ -394,7 +400,7 @@ void add_to_alternates_file(const char *reference)
 	if (commit_lock_file(lock))
 		die("could not close alternates file");
 	if (alt_odb_tail)
-		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
+		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0, 0);
 }
 
 void foreach_alt_odb(alt_odb_fn fn, void *cb)

@@ -418,9 +424,9 @@ void prepare_alt_odb(void)
 	if (!alt) alt = "";
 
 	alt_odb_tail = &alt_odb_list;
-	link_alt_odb_entries(alt, alt + strlen(alt), PATH_SEP, NULL, 0);
+	link_alt_odb_entries(alt, alt + strlen(alt), PATH_SEP, NULL, 0, 0);
 
-	read_info_alternates(get_object_directory(), 0);
+	read_info_alternates(get_object_directory(), 0, 0);
 }
 
 static int has_loose_object_local(const unsigned char *sha1)

@@ -2573,3 +2579,11 @@ int read_pack_header(int fd, struct pack_header *header)
 		return PH_ERROR_PROTOCOL;
 	return 0;
 }
+
+int add_alt_odb(char *path, int quiet)
+{
+	int err = link_alt_odb_entry(path, strlen(path), NULL, 0, quiet);
+	if (!err)
+		prepare_packed_git_one(path, 0);
+	return err;
+}

-- 
1.6.1.150.g5e733b

The new --submodules option is used to trigger inclusion of checked
out submodules in the archive.

The implementation currently does not verify that the submodule has
been registered as 'interesting' in .git/config, neither does it resolve
the currently checked out submodule HEAD but instead uses the commit SHA1
recorded in the gitlink entry to identify the submodule root tree.

The plan is to fix these limitations by extending --submodules to allow
certain flags/options:
  a|c|r     include any|checked out|registered submodules
  H         resolve submodule HEAD to decide which tree to include
  g:<name>  only include submodules in group <name>

The syntax would then become '--submodules[=[a|c|r][H][g:<name>]]' and
group membership could be specified in .git/config and/or .gitmodules.
The current behavior would then match '--submodules=c' (which might be a
sensible default when only --submodules is specified).

Signed-off-by: Lars Hjemli <redacted>
---
 Documentation/git-archive.txt |    3 +
 archive.c                     |   53 ++++++++++++++++++-
 archive.h                     |    1 +
 t/t5001-archive-submodules.sh |  121 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 177 insertions(+), 1 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 41cbf9c..ddfa343 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt

@@ -51,6 +51,9 @@ OPTIONS
 	This can be any options that the archiver backend understand.
 	See next section.
 
+--submodules::
+	Include all checked out submodules in the archive.
+
 --remote=<repo>::
 	Instead of making a tar archive from local repository,
 	retrieve a tar archive from a remote repository.

diff --git a/archive.c b/archive.c
index e6de039..1709a01 100644
--- a/archive.c
+++ b/archive.c

@@ -96,6 +96,52 @@ struct archiver_context {
 	write_archive_entry_fn_t write_entry;
 };
 
+/* Given the root directory of a non-bare repository, return the path
+ * to the corresponding GITDIR, or NULL if not found. The return-value
+ * is malloc'd by this function and should be free'd by the caller.
+ */
+static char *get_gitdir(const char *root)
+{
+	const char *path, *tmp;
+	struct stat st;
+
+	if (!root)
+		return NULL;
+
+	if (root[strlen(root) - 1] == '/')
+		path = mkpath("%s.git", root);
+	else
+		path = mkpath("%s/.git", root);
+
+	tmp = read_gitfile_gently(path);
+	if (tmp)
+		path = tmp;
+
+	if (stat(path, &st) || !S_ISDIR(st.st_mode))
+		return NULL;
+	return xstrdup(path);
+}
+
+/* Return READ_TREE_RECURSIVE if we should recurse into the gitlinked
+ * repository or 0 if it should be skipped.
+ */
+static int recurse_gitlink(struct archiver_args *args, const char *path)
+{
+	char *gitdir;
+	char *objdir;
+
+	if (!args->submodules)
+		return 0;
+	gitdir = get_gitdir(path);
+	if (!gitdir)
+		return 0;
+	objdir = mkpath("%s/objects", gitdir);
+	free(gitdir);
+	if (add_alt_odb(objdir, 0))
+		return -1;
+	return READ_TREE_RECURSIVE;
+}
+
 static int write_archive_entry(const unsigned char *sha1, const char *base,
 		int baselen, const char *filename, unsigned mode, int stage,
 		void *context)

@@ -132,7 +178,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 :
+					recurse_gitlink(args, path.buf));
 	}
 
 	buffer = sha1_file_to_archive(path_without_prefix, sha1, mode,

@@ -255,6 +302,7 @@ static int parse_archive_args(int argc, const char **argv,
 	const char *exec = NULL;
 	int compression_level = -1;
 	int verbose = 0;
+	int submodules = 0;
 	int i;
 	int list = 0;
 	struct option opts[] = {

@@ -262,6 +310,8 @@ 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"),
+		OPT_BOOLEAN(0, "submodules", &submodules,
+			"include checked out submodules in the archive"),
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),

@@ -319,6 +369,7 @@ static int parse_archive_args(int argc, const char **argv,
 	args->verbose = verbose;
 	args->base = base;
 	args->baselen = strlen(base);
+	args->submodules = submodules;
 
 	return argc;
 }

diff --git a/archive.h b/archive.h
index 0b15b35..aff3fcd 100644
--- a/archive.h
+++ b/archive.h

@@ -11,6 +11,7 @@ struct archiver_args {
 	const char **pathspec;
 	unsigned int verbose : 1;
 	int compression_level;
+	int submodules;
 };
 
 typedef int (*write_archive_fn_t)(struct archiver_args *);

diff --git a/t/t5001-archive-submodules.sh b/t/t5001-archive-submodules.sh
new file mode 100755
index 0000000..6471984
--- /dev/null
+++ b/t/t5001-archive-submodules.sh

@@ -0,0 +1,121 @@
+#!/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, all submodules are ignored' '
+	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_debug 'tar -tf normal.tar'
+
+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_debug 'tar -tf full.tar'
+
+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_debug 'tar -tf recursive.tar'
+
+test_expect_success 'packed submodules are supported' '
+	cat <<EOF >expected &&
+1
+2/
+2/3
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	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_debug 'tar -tf packed.tar'
+
+test_expect_success 'a missing submodule pack triggers an error' '
+	find 2/.git/objects/pack -type f | xargs rm &&
+	test_must_fail git archive --submodules HEAD
+'
+
+test_expect_success 'non-checked out submodules are ignored' '
+	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_debug 'tar -tf partial.tar'
+
+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

When the callback function invoked from read_tree_recursive() returns
`READ_TREE_RECURSIVE` for a gitlink entry, the traversal will now
continue into the tree connected to the gitlinked commit. It is the
responsibility of the callback function to somehow make the gitlinked
commit (and corresponding tree/blob) objects available, possibly by
inserting the submodule object database as an alternate odb.

Also, all existing callback function has been updated to only return
READ_TREE_RECURSIVE for directory entries, so this patch should not
introduce any changes to current behavior.

Signed-off-by: Lars Hjemli <redacted>
---
 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

Hi,

On Thu, 22 Jan 2009, Lars Hjemli wrote:

FWIW my concern is not at all addressed.  A future user of add_alt_odb() 
(and possibly your users in rare cases, too) can trigger the error that 
suggests looking into the alternates.  Leaving the human user puzzled.

Ciao,
Dscho

Hi,

On Thu, 22 Jan 2009, Lars Hjemli wrote:

The new --submodules option is used to trigger inclusion of checked out 
submodules in the archive.

The implementation currently does not verify that the submodule has been 
registered as 'interesting' in .git/config, neither does it resolve the 
currently checked out submodule HEAD but instead uses the commit SHA1 
recorded in the gitlink entry to identify the submodule root tree.

Please understand that I skipped the rest of the patch.

Ciao,
Dscho

On Fri, Jan 23, 2009 at 00:43, Johannes Schindelin
[off-list ref] wrote:

On Thu, 22 Jan 2009, Lars Hjemli wrote:

quoted

+             if (!quiet)
+                     error("object directory %s does not exist; "
+                           "check .git/objects/info/alternates.",
+                           ent->base);

FWIW my concern is not at all addressed.  A future user of add_alt_odb()
(and possibly your users in rare cases, too) can trigger the error that
suggests looking into the alternates.  Leaving the human user puzzled.

Is it the phrasing of the error message that concerns you (when
invoked from add_alt_odb())?

If so, would something like this be ok/better?

quoted

+             if (!quiet)
+                     error("Alternate object directory %s does not exist ",
+                           ent->base);

--
larsh

On Fri, Jan 23, 2009 at 00:44, Johannes Schindelin
[off-list ref] wrote:

On Thu, 22 Jan 2009, Lars Hjemli wrote:

quoted

The new --submodules option is used to trigger inclusion of checked out
submodules in the archive.

The implementation currently does not verify that the submodule has been
registered as 'interesting' in .git/config, neither does it resolve the
currently checked out submodule HEAD but instead uses the commit SHA1
recorded in the gitlink entry to identify the submodule root tree.

Please understand that I skipped the rest of the patch.

That's too bad, I hoped on some feedback from you on the part of the
commit message which you didn't quote:

quoted

The plan is to fix these limitations by extending --submodules to allow
certain flags/options:
a|c|r     include any|checked out|registered submodules
H         resolve submodule HEAD to decide which tree to include
g:<name>  only include submodules in group <name>

The syntax would then become '--submodules[=[a|c|r][H][g:<name>]]' and
group membership could be specified in .git/config and/or .gitmodules.
The current behavior would then match '--submodules=c' (which might be a
sensible default when only --submodules is specified).

Wouldn't such an option address your concern about the
consistency/semantics of the --submodules operation?

--
larsh

Hi,

On Fri, 23 Jan 2009, Lars Hjemli wrote:

On Fri, Jan 23, 2009 at 00:43, Johannes Schindelin
[off-list ref] wrote:

quoted

On Thu, 22 Jan 2009, Lars Hjemli wrote:

quoted

+             if (!quiet)
+                     error("object directory %s does not exist; "
+                           "check .git/objects/info/alternates.",
+                           ent->base);

FWIW my concern is not at all addressed.  A future user of add_alt_odb()
(and possibly your users in rare cases, too) can trigger the error that
suggests looking into the alternates.  Leaving the human user puzzled.

Is it the phrasing of the error message that concerns you (when
invoked from add_alt_odb())?

If so, would something like this be ok/better?

quoted

quoted

+             if (!quiet)
+                     error("Alternate object directory %s does not exist ",
+                           ent->base);

That would almost certainly be better.

Ciao,
Dscho

Hi,

On Fri, 23 Jan 2009, Lars Hjemli wrote:

On Fri, Jan 23, 2009 at 00:44, Johannes Schindelin
[off-list ref] wrote:

quoted

On Thu, 22 Jan 2009, Lars Hjemli wrote:

quoted

The new --submodules option is used to trigger inclusion of checked out
submodules in the archive.

The implementation currently does not verify that the submodule has 
been registered as 'interesting' in .git/config, neither does it 
resolve the currently checked out submodule HEAD but instead uses the 
commit SHA1 recorded in the gitlink entry to identify the submodule 
root tree.

Please understand that I skipped the rest of the patch.

That's too bad, I hoped on some feedback from you on the part of the 
commit message which you didn't quote:

Well, you ignored my comments, so what do you expect me to do?  Be happy?

There are two issues there:

- presence of a specific commit object being present in the repository 
  does not necessarily mean that it is reachable by any ref, and therefore 
  can mean that the tree/blob objects are not reachable, because it could 
  be an interrupted fetch; in all of Git, we try to assume that only 
  reachable objects are valid objects.

- presence of a specific commit in the supermodule is a _lousy_ indicator 
  that the user wants to include that submodule in the archive.

Until both issues are addresse, I will not dance a little song and be 
merry over this issue.

Ciao,
Dscho

On Fri, Jan 23, 2009 at 20:57, Johannes Schindelin
[off-list ref] wrote:

On Fri, 23 Jan 2009, Lars Hjemli wrote:

quoted

That's too bad, I hoped on some feedback from you on the part of the
commit message which you didn't quote:

Well, you ignored my comments,

I might have misunderstood your comments, but I certainly didn't
ignore them. I actually tried to come up with a solution that would
solve your concerns about which submodules to include in the archive
(which is why I hoped for some feedback on that proposal).

so what do you expect me to do?  Be happy?

There are two issues there:

- presence of a specific commit object being present in the repository
 does not necessarily mean that it is reachable by any ref, and therefore
 can mean that the tree/blob objects are not reachable, because it could
 be an interrupted fetch;

This part I agree with.

 in all of Git, we try to assume that only
 reachable objects are valid objects.

I don't think this is true (most git commands accepts their arguments
as valid objects without verifying if they are reachable from a ref).
Do you feel it is necessary to perform a reachability check of the
gitlink'd commit before traversing into a submodule tree?

- presence of a specific commit in the supermodule is a _lousy_ indicator
 that the user wants to include that submodule in the archive.

This is the issue I tried to address with my
`--submodules=[a|c|r][g:<name>]` proposal in the commit message for
this patch. I hoped you would find it interesting, given your comments
in http://thread.gmane.org/gmane.comp.version-control.git/106167/focus=106235
(i.e. my 'a' flag would match your 'look-in-superprojects-odb', while
the 'c', 'r' and 'g' options would address your issues about how to
select the correct set of submodules).

--
larsh

Hi,

On Sat, 24 Jan 2009, Lars Hjemli wrote:

On Fri, Jan 23, 2009 at 20:57, Johannes Schindelin
[off-list ref] wrote:

quoted

 in all of Git, we try to assume that only reachable objects are valid 
 objects.

I don't think this is true (most git commands accepts their arguments
as valid objects without verifying if they are reachable from a ref).

The fact that a user can ask for some object directly, and that we do not 
try to validate it in that case has nothing to do with said assumption.

If something is pushed to a remote, and the connection fails, some commit 
could be pushed already, but some of its reachable objects lacking.

The user on the remote side can still try to salvage parts by accessing 
the objects directly, by their name.

But the only guarantee that the objects are reachable is to start from a 
ref.

Concretely, if your patch is applied as-is, such a half-pushed state could 
affect git-archive in a nasty way: even if the user started from a ref, 
there could be missing objects!

Do you feel it is necessary to perform a reachability check of the 
gitlink'd commit before traversing into a submodule tree?

No.  Because HEAD is a ref, too.

Now, there is still a problem when your submodule is missing the objects 
for the commit your superproject is referring to.

IMO that is a serious issue, as it just asks for confused users.

quoted

- presence of a specific commit in the supermodule is a _lousy_ 
  indicator that the user wants to include that submodule in the 
  archive.

This is the issue I tried to address with my
`--submodules=[a|c|r][g:<name>]` proposal in the commit message for
this patch.

Nope, doing this "in the future" does not please me one bit.

Besides, I find the semantics, uhm, "interesting".  (The other word would 
be "unintuitive".  Why do you have to be so cryptic that I have to read 
the proposal to understand what the heck "c" is about?)

Ciao,
Dscho

On Sat, Jan 24, 2009 at 14:51, Johannes Schindelin
[off-list ref] wrote:

Now, there is still a problem when your submodule is missing the objects
for the commit your superproject is referring to.

IMO that is a serious issue, as it just asks for confused users.

This made me finally understand your concern (sorry for being slow):
you want the command to behave in a predictable/consistent way while
my implementation would end up making an archive with basically random
content.

quoted

quoted

- presence of a specific commit in the supermodule is a _lousy_
  indicator that the user wants to include that submodule in the
  archive.

This is the issue I tried to address with my
`--submodules=[a|c|r][g:<name>]` proposal in the commit message for
this patch.

Nope, doing this "in the future" does not please me one bit.

Besides, I find the semantics, uhm, "interesting".  (The other word would
be "unintuitive".  Why do you have to be so cryptic that I have to read
the proposal to understand what the heck "c" is about?)

I thought it would be nifty to be able to combine different flags
which would affect the behaviour/semantics of the command, but given
the comments from you and Junio, I think I'll end up with something
like this:

$ git archive --submodules <tree-ish>: Create an archive which
includes the trees of all gitlink entries in <tree-ish>, fail unless
all the required objects are available.

$ git archive --submodules=<group>: Same as above, but only traverse
submodules in the specified group (as defined in $GIT_CONFIG).

--
larsh

Hi,

On Sat, 24 Jan 2009, Lars Hjemli wrote:

$ git archive --submodules <tree-ish>: Create an archive which
includes the trees of all gitlink entries in <tree-ish>, fail unless
all the required objects are available.

$ git archive --submodules=<group>: Same as above, but only traverse
submodules in the specified group (as defined in $GIT_CONFIG).

How about having the former with --submodules='*' and let --submodules 
without argument include those submodules that are checked out (none in a 
bare repository)?

Thanks,
Dscho

On Sat, Jan 24, 2009 at 20:52, Johannes Schindelin
[off-list ref] wrote:

Hi,

On Sat, 24 Jan 2009, Lars Hjemli wrote:

quoted

$ git archive --submodules <tree-ish>: Create an archive which
includes the trees of all gitlink entries in <tree-ish>, fail unless
all the required objects are available.

$ git archive --submodules=<group>: Same as above, but only traverse
submodules in the specified group (as defined in $GIT_CONFIG).

How about having the former with --submodules='*' and let --submodules
without argument include those submodules that are checked out (none in a
bare repository)?

Yeah, that might make more sense (since you'd normally not have access
to the content of non-checked out submodules). I'm also considering
something like --submodules[=(all|checkedout|[group:]<name>)], i.e.
the 'group:'-part could be optional as long as <name> is unambiguous.

--
larsh