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

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

[Lars Hjemli]

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

[RFC/PATCH v3 1/3] tree.c: teach read_tree_recursive how to traverse gitlink entries

[Lars Hjemli]

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

[RFC/PATCH v3 2/3] sha1_file: prepare for adding alternates on demand

[Lars Hjemli]

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 <hjemli@gmail.com>
---
 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

[RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

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 <hjemli@gmail.com>
---
 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

Re: [RFC/PATCH v3 2/3] sha1_file: prepare for adding alternates on demand

[Johannes Schindelin]

Hi,

On Thu, 22 Jan 2009, Lars Hjemli wrote:

> @@ -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;
>  	}
> [...]
> @@ -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;
> +}

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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Johannes Schindelin]

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

Re: [RFC/PATCH v3 2/3] sha1_file: prepare for adding alternates on demand

[Lars Hjemli]

On Fri, Jan 23, 2009 at 00:43, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 22 Jan 2009, Lars Hjemli wrote:
>> +             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?

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

--
larsh

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Fri, Jan 23, 2009 at 00:44, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.

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

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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

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

What do you mean by "decide"?  If HEAD exists (iow, the submodule is
checked out), the tree of the commit recorded in the superproject's
gitlink entry is included in the result?

As I already said before, I doubt it makes much sense in the context of
the current git-archive to base the choise on checkout status.

Unless you are extending git-archive and giving it an ability to write out
the superproject index or the work tree as an archive, that is.

Just like git-grep lets you grep in the work tree files (limited to paths
that appear in the index), or grep in the contents registered to the index
when run with --cached, git-archive could make an archive out of your work
tree files or your index contents.  Such an extension to git-archive may
be quite useful with or without submodules.

In such mode of operation, because you are dealing with the work tree when
run without --cached, it would make sense to say "Ah, the superproject
index wants v1.0 of the submodule, but the work tree has v2.0 of it
checked out, and we are writing out the work tree, so let's include v2.0
instead", and as a side effect of deciding which commit's tree to include
from each submodule, it naturally makes sense to exclude submodules that
are not checked out.

But otherwise I am not so sure what the point of H option would be.

Re: [RFC/PATCH v3 2/3] sha1_file: prepare for adding alternates on demand

[Johannes Schindelin]

Hi,

On Fri, 23 Jan 2009, Lars Hjemli wrote:

> On Fri, Jan 23, 2009 at 00:43, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Thu, 22 Jan 2009, Lars Hjemli wrote:
> >> +             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?
> 
> >> +             if (!quiet)
> >> +                     error("Alternate object directory %s does not exist ",
> >> +                           ent->base);

That would almost certainly be better.

Ciao,
Dscho

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Johannes Schindelin]

Hi,

On Fri, 23 Jan 2009, Lars Hjemli wrote:

> On Fri, Jan 23, 2009 at 00:44, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > 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.
> 
> 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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Fri, Jan 23, 2009 at 20:23, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>>>> 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
>
> What do you mean by "decide"?  If HEAD exists (iow, the submodule is
> checked out), the tree of the commit recorded in the superproject's
> gitlink entry is included in the result?

No, when H is specified the tree of the currently checked out
submodule commit would be included (this obviously shouldn't be the
default mode of operation, hence a flag to trigger it if that is what
the user wants).

>
> As I already said before, I doubt it makes much sense in the context of
> the current git-archive to base the choise on checkout status.
>
> Unless you are extending git-archive and giving it an ability to write out
> the superproject index or the work tree as an archive, that is.
>
> Just like git-grep lets you grep in the work tree files (limited to paths
> that appear in the index), or grep in the contents registered to the index
> when run with --cached, git-archive could make an archive out of your work
> tree files or your index contents.  Such an extension to git-archive may
> be quite useful with or without submodules.

Absolutely.

>
> In such mode of operation, because you are dealing with the work tree when
> run without --cached, it would make sense to say "Ah, the superproject
> index wants v1.0 of the submodule, but the work tree has v2.0 of it
> checked out, and we are writing out the work tree, so let's include v2.0
> instead", and as a side effect of deciding which commit's tree to include
> from each submodule, it naturally makes sense to exclude submodules that
> are not checked out.
>
> But otherwise I am not so sure what the point of H option would be.

I would find the H flag practical for my own usage of submodules. I
almost never modify the content of the currently checked out submodule
but I often check out a different HEAD than what is registered in the
gitlink in the superproject (typically due to testing the superproject
against different versions of the submodule). And for such a use case,
being able to create a tarball of my currently checked out state seems
useful to me.

Anyways, if we get as far as adding a --submodules option to git
archive, I believe its default mode should be to archive the
superproject HEAD state together with the gitlink'd state of each
submodule registered in .git/config instead of --submodules=c which is
what this patch implements. But I wanted to get some feedback on this
plan before trying to implement it.

--
larsh

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

> On Fri, Jan 23, 2009 at 20:23, Junio C Hamano <gitster@pobox.com> wrote:
>> Lars Hjemli <hjemli@gmail.com> writes:
>>
>>>>> 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
>>
>> What do you mean by "decide"?  If HEAD exists (iow, the submodule is
>> checked out), the tree of the commit recorded in the superproject's
>> gitlink entry is included in the result?
>
> No, when H is specified the tree of the currently checked out
> submodule commit would be included.

That makes even less sense.  At that point you are mixing a tree with
random state from a work tree, and doing so only for submodules.  If you
want a work tree snapshot, it should be a work tree snapshot, and should
not be labelled as a snapshot out of a tree object of the superproject.

> I would find the H flag practical for my own usage of submodules. I
> almost never modify the content of the currently checked out submodule
> but I often check out a different HEAD than what is registered in the
> gitlink in the superproject (typically due to testing the superproject
> against different versions of the submodule). And for such a use case,
> being able to create a tarball of my currently checked out state seems
> useful to me.

That would be more like an enhanced version of "git archive" that takes
the work tree state, similar to how "git grep" operates on the work tree
today.

I agree that would be useful, but I have a moderately strong suspition
that your "H" hack that includes the work tree state for checked out
submodules into a view that is primarily about the "tree" object in the
superproject, without the same "take from the work tree" semantics for
paths in the superproject, is more harmful than being helpful to the users
in the longer term.  It might be simple to implement, but I do not think
its semantics can be explained sanely.

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Fri, Jan 23, 2009 at 21:50, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>> I would find the H flag practical for my own usage of submodules. I
>> almost never modify the content of the currently checked out submodule
>> but I often check out a different HEAD than what is registered in the
>> gitlink in the superproject (typically due to testing the superproject
>> against different versions of the submodule). And for such a use case,
>> being able to create a tarball of my currently checked out state seems
>> useful to me.
>
> That would be more like an enhanced version of "git archive" that takes
> the work tree state, similar to how "git grep" operates on the work tree
> today.
>
> I agree that would be useful, but I have a moderately strong suspition
> that your "H" hack that includes the work tree state for checked out
> submodules into a view that is primarily about the "tree" object in the
> superproject, without the same "take from the work tree" semantics for
> paths in the superproject, is more harmful than being helpful to the users
> in the longer term.  It might be simple to implement, but I do not think
> its semantics can be explained sanely.

Ok. When someone gets the itch for 'git archive --worktree', it would
be easy enough to resurrect the idea of including the tree of the
currently checked out HEAD in selected submodules.

Do you think the other options makes any sense, i.e. is it any point
in me trying to implement this?

--
larsh

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Fri, Jan 23, 2009 at 20:57, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Fri, 23 Jan 2009, Lars Hjemli wrote:
>>
>> 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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Johannes Schindelin]

Hi,

On Sat, 24 Jan 2009, Lars Hjemli wrote:

> On Fri, Jan 23, 2009 at 20:57, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> >  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.

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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Sat, Jan 24, 2009 at 14:51, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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.


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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Johannes Schindelin]

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

Re: [RFC/PATCH v3 3/3] archive.c: add basic support for submodules

[Lars Hjemli]

On Sat, Jan 24, 2009 at 20:52, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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)?

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