[WIP PATCH 0/4] Recursively checkout submodules

[WIP PATCH 0/4] Recursively checkout submodules

[Jens Lehmann]

Having found some Git time recently i started to tackle the next
major issue on my agenda to improve submodule experience: Check
out the matching versions of populated submodules when doing a
checkout in the superproject (without having to do an explicit
"git submodule update").

So here is what i came up with so far: The first two patches lay
the groundwork by introducing a new function checkout_submodule()
in submodule.c and calling it from checkout_entry(). The new
"ignore_submodules" flags added to "struct checkout" and "struct
unpack_trees_options" can be used to prevent that (and default
to true, which is the old behavior then changed by the following
patches). The next two patches teach "git checkout" and "git
checkout-index" to default to checking out submodules too unless
the new option "--ignore-subodules" is used.

What's working:
* Changing branches and specific revisions update the submodules
  accordingly
* "Revert changes" in "git gui" resets submodules now

What's missing:
* Test cases
* I think "git fetch" should recurse into populated submodules
  too, otherwise the commits to check out there might be missing


Opinions?


Jens Lehmann (4):
  Prepare checkout_entry() for recursive checkout of submodules
  Add "ignore_submodules" member to "struct unpack_trees_options"
  Teach checkout to recursively checkout submodules
  Teach checkout-index to recursively checkout submodules

 Documentation/git-checkout-index.txt |    9 ++++++++-
 Documentation/git-checkout.txt       |    7 +++++++
 archive.c                            |    1 +
 builtin/apply.c                      |    1 +
 builtin/checkout-index.c             |    5 ++++-
 builtin/checkout.c                   |    6 ++++++
 builtin/clone.c                      |    1 +
 builtin/commit.c                     |    1 +
 builtin/merge.c                      |    2 ++
 builtin/read-tree.c                  |    1 +
 builtin/reset.c                      |    1 +
 cache.h                              |    3 ++-
 diff-lib.c                           |    2 ++
 entry.c                              |    9 ++++++---
 merge-recursive.c                    |    1 +
 submodule.c                          |   32 ++++++++++++++++++++++++++++++++
 submodule.h                          |    1 +
 t/t2013-checkout-submodule.sh        |   14 ++++++++++++--
 unpack-trees.c                       |    1 +
 unpack-trees.h                       |    3 ++-
 20 files changed, 92 insertions(+), 9 deletions(-)

[WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Jens Lehmann]

To achieve this, the new member "ignore_submodules" has been added to the
bitfield in "struct checkout". All users of "struct checkout" are setting
this flag to 1 for backward compatibility for now. Also the new function
checkout_submodule() has been added and is called by checkout_entry() when
"ignore_submodules" is false (which will be the default behavior later).

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 builtin/apply.c          |    1 +
 builtin/checkout-index.c |    1 +
 builtin/checkout.c       |    1 +
 cache.h                  |    3 ++-
 entry.c                  |    9 ++++++---
 submodule.c              |   32 ++++++++++++++++++++++++++++++++
 submodule.h              |    1 +
 unpack-trees.c           |    1 +
 8 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index ec755ac..a7788a9 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2827,6 +2827,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 			costate.quiet = 0;
 			costate.not_new = 0;
 			costate.refresh_cache = 1;
+			costate.ignore_submodules = 1;
 			if (checkout_entry(*ce, &costate, NULL) ||
 			    lstat(old_name, st))
 				return -1;
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a7a5ee1..c35bad3 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -254,6 +254,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	state.force = force;
 	state.quiet = quiet;
 	state.not_new = not_new;
+	state.ignore_submodules = 1;

 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c382521..ee198ae 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -248,6 +248,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	memset(&state, 0, sizeof(state));
 	state.force = 1;
 	state.refresh_cache = 1;
+	state.ignore_submodules = 1;
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {
diff --git a/cache.h b/cache.h
index 303c107..996fa69 100644
--- a/cache.h
+++ b/cache.h
@@ -802,7 +802,8 @@ struct checkout {
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
-		 refresh_cache:1;
+		 refresh_cache:1,
+		 ignore_submodules:1;
 };

 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
diff --git a/entry.c b/entry.c
index 004182c..15dec09 100644
--- a/entry.c
+++ b/entry.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "blob.h"
 #include "dir.h"
+#include "submodule.h"

 static void create_directories(const char *path, int path_len,
 			       const struct checkout *state)
@@ -222,9 +223,11 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 		 * just do the right thing)
 		 */
 		if (S_ISDIR(st.st_mode)) {
-			/* If it is a gitlink, leave it alone! */
-			if (S_ISGITLINK(ce->ce_mode))
-				return 0;
+			if (S_ISGITLINK(ce->ce_mode)) {
+				if (state->ignore_submodules)
+					return 0;
+				return checkout_submodule(ce->name, ce->sha1, state->force);
+	       		}
 			if (!state->force)
 				return error("%s is a directory", path);
 			remove_subtree(path);
diff --git a/submodule.c b/submodule.c
index b3b8bc1..59029b2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -206,3 +206,35 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	strbuf_release(&buf);
 	return dirty_submodule;
 }
+
+int checkout_submodule(const char *path, const unsigned char sha1[20], int force)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process cp;
+	const char *hex_sha1 = sha1_to_hex(sha1);
+	const char *argv[] = {
+		"checkout",
+                force ? "-qf" : "-q",
+		hex_sha1,
+		NULL,
+	};
+	
+	strbuf_addf(&buf, "%s/.git/", path);
+	if (!is_directory(buf.buf)) {
+		strbuf_release(&buf);
+		/* The submodule is not populated, so we can't check it out */
+		return 0;
+	}
+
+	memset(&cp, 0, sizeof(cp));
+	cp.argv = argv;
+	cp.env = local_repo_env;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.out = -1;
+	cp.dir = path;   /* GIT_WORK_TREE doesn't work for git checkout */
+	if (run_command(&cp))
+		return error("Could not checkout submodule %s", path);
+
+	return 0;
+}
diff --git a/submodule.h b/submodule.h
index dbda270..fc6909e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -6,5 +6,6 @@ void show_submodule_summary(FILE *f, const char *path,
 		unsigned dirty_submodule,
 		const char *del, const char *add, const char *reset);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
+int checkout_submodule(const char *path, const unsigned char sha1[20], int force);

 #endif
diff --git a/unpack-trees.c b/unpack-trees.c
index c29a9e0..151b422 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -711,6 +711,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	state.force = 1;
 	state.quiet = 1;
 	state.refresh_cache = 1;
+	state.ignore_submodules = 1;

 	memset(&el, 0, sizeof(el));
 	if (!core_apply_sparse_checkout || !o->update)
-- 
1.7.1.rc0.248.g09203

[WIP PATCH 2/4] Add "ignore_submodules" member to "struct unpack_trees_options"

[Jens Lehmann]

To be able to checkout submodule content the new member "ignore_submodules"
is added to "struct unpack_trees_options". In "unpack_callback()" it is
used to initialize the recently introduced member with the same name in
"struct checkout".

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 archive.c           |    1 +
 builtin/clone.c     |    1 +
 builtin/commit.c    |    1 +
 builtin/merge.c     |    2 ++
 builtin/read-tree.c |    1 +
 builtin/reset.c     |    1 +
 diff-lib.c          |    2 ++
 merge-recursive.c   |    1 +
 unpack-trees.c      |    2 +-
 unpack-trees.h      |    3 ++-
 10 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/archive.c b/archive.c
index d700af3..d730bab 100644
--- a/archive.c
+++ b/archive.c
@@ -184,6 +184,7 @@ int write_archive_entries(struct archiver_args *args,
 		opts.src_index = &the_index;
 		opts.dst_index = &the_index;
 		opts.fn = oneway_merge;
+		opts.ignore_submodules = 1;
 		init_tree_desc(&t, args->tree->buffer, args->tree->size);
 		if (unpack_trees(1, &t, &opts))
 			return -1;
diff --git a/builtin/clone.c b/builtin/clone.c
index 05be999..f826fe9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -637,6 +637,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		opts.verbose_update = (option_verbosity > 0);
 		opts.src_index = &the_index;
 		opts.dst_index = &the_index;
+		opts.ignore_submodules = 1;

 		tree = parse_tree_indirect(our_head_points_at->old_sha1);
 		parse_tree(tree);
diff --git a/builtin/commit.c b/builtin/commit.c
index 017b8a5..419a4cf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -257,6 +257,7 @@ static void create_base_index(void)
 	opts.dst_index = &the_index;

 	opts.fn = oneway_merge;
+	opts.ignore_submodules = 1;
 	tree = parse_tree_indirect(head_sha1);
 	if (!tree)
 		die("failed to unpack HEAD tree object");
diff --git a/builtin/merge.c b/builtin/merge.c
index 37d414b..e589404 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -532,6 +532,7 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,
 	if (!trees[nr_trees++])
 		return -1;
 	opts.fn = threeway_merge;
+	opts.ignore_submodules = 1;
 	cache_tree_free(&active_cache_tree);
 	for (i = 0; i < nr_trees; i++) {
 		parse_tree(trees[i]);
@@ -705,6 +706,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	opts.merge = 1;
 	opts.fn = twoway_merge;
 	opts.msgs = get_porcelain_error_msgs();
+	opts.ignore_submodules = 1;

 	trees[nr_trees] = parse_tree_indirect(head);
 	if (!trees[nr_trees++])
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 8bdcab1..cd3edfa 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -201,6 +201,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)

 	if (opts.debug_unpack)
 		opts.fn = debug_merge;
+	opts.ignore_submodules = 1;

 	cache_tree_free(&active_cache_tree);
 	for (i = 0; i < nr_trees; i++) {
diff --git a/builtin/reset.c b/builtin/reset.c
index 1283068..0ac7e6f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -98,6 +98,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 		nr++;
 		opts.fn = twoway_merge;
 	}
+	opts.ignore_submodules = 1;

 	if (!fill_tree_descriptor(desc + nr - 1, sha1))
 		return error("Failed to find tree of %s.", sha1_to_hex(sha1));
diff --git a/diff-lib.c b/diff-lib.c
index c9f6e05..64264a9 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -451,6 +451,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 	opts.unpack_data = revs;
 	opts.src_index = &the_index;
 	opts.dst_index = NULL;
+	opts.ignore_submodules = 1;

 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
@@ -509,6 +510,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	opts.unpack_data = &revs;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	opts.ignore_submodules = 1;

 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
diff --git a/merge-recursive.c b/merge-recursive.c
index 206c103..d294e62 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -186,6 +186,7 @@ static int git_merge_trees(int index_only,
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	opts.msgs = get_porcelain_error_msgs();
+	opts.ignore_submodules = 1;

 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);
diff --git a/unpack-trees.c b/unpack-trees.c
index 151b422..6821d7d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -711,7 +711,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	state.force = 1;
 	state.quiet = 1;
 	state.refresh_cache = 1;
-	state.ignore_submodules = 1;
+	state.ignore_submodules = o->ignore_submodules;

 	memset(&el, 0, sizeof(el));
 	if (!core_apply_sparse_checkout || !o->update)
diff --git a/unpack-trees.h b/unpack-trees.h
index ef70eab..bbb49fd 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,7 +33,8 @@ struct unpack_trees_options {
 		     diff_index_cached,
 		     debug_unpack,
 		     skip_sparse_checkout,
-		     gently;
+		     gently,
+		     ignore_submodules;
 	const char *prefix;
 	int cache_bottom;
 	struct dir_struct *dir;
-- 
1.7.1.rc0.248.g09203

[WIP PATCH 3/4] Teach checkout to recursively checkout submodules

[Jens Lehmann]

Change the default behavior of "git checkout" to check out submodules
too. This can be prevented by using the new "--ignore-submodules" option.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/git-checkout.txt |    7 +++++++
 builtin/checkout.c             |    7 ++++++-
 t/t2013-checkout-submodule.sh  |   14 ++++++++++++--
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 4505eb6..63ac3bc 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -144,6 +144,13 @@ the conflicted merge in the specified paths.
 This means that you can use `git checkout -p` to selectively discard
 edits from your current working tree.

+--ignore-submodules::
+	Since version 1.8.0 the default behavior is to checkout populated
+	submodules recursively. When this option is used, the work trees of
+	submodules will not be updated, only the hash recorded in the
+	superproject will be changed (this was the default behavior until
+	1.8.0).
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ee198ae..0008ec5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -31,6 +31,7 @@ struct checkout_opts {
 	int force;
 	int writeout_stage;
 	int writeout_error;
+	int ignore_submodules;

 	const char *new_branch;
 	const char *new_orphan_branch;
@@ -248,7 +249,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	memset(&state, 0, sizeof(state));
 	state.force = 1;
 	state.refresh_cache = 1;
-	state.ignore_submodules = 1;
+	state.ignore_submodules = opts->ignore_submodules;
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {
@@ -314,6 +315,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 	opts.verbose_update = !o->quiet;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	opts.ignore_submodules = o->ignore_submodules;
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
@@ -393,6 +395,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
 		topts.dir->exclude_per_dir = ".gitignore";
+		topts.ignore_submodules = opts->ignore_submodules;
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
@@ -649,6 +652,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		{ OPTION_BOOLEAN, 0, "guess", &dwim_new_local_branch, NULL,
 		  "second guess 'git checkout no-such-branch'",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
+		OPT_BOOLEAN(0, "ignore-submodules", &opts.ignore_submodules,
+			    "don't update submodule work trees"),
 		OPT_END(),
 	};
 	int has_dash_dash;
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index fda3f0a..e5f2e6e 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -29,14 +29,24 @@ test_expect_success '"reset <submodule>" updates the index' '
 	git diff-files --quiet
 '

-test_expect_success '"checkout <submodule>" updates the index only' '
+test_expect_success '"checkout --ignore-submodules <submodule>" updates the index only' '
 	git update-index --refresh &&
 	git diff-files --quiet &&
 	git diff-index --quiet --cached HEAD &&
-	git checkout HEAD^ submodule &&
+	git checkout --ignore-submodules HEAD^ submodule &&
 	test_must_fail git diff-files --quiet &&
 	git checkout HEAD submodule &&
 	git diff-files --quiet
 '

+test_expect_success '"checkout <submodule>" updates recursively' '
+	git update-index --refresh &&
+	git diff-files --quiet &&
+	git diff-index --quiet --cached HEAD &&
+	git checkout HEAD^ submodule &&
+	git diff-files --quiet &&
+	git checkout HEAD submodule &&
+	git diff-files --quiet
+'
+
 test_done
-- 
1.7.1.rc0.248.g09203

[WIP PATCH 4/4] Teach checkout-index to recursively checkout submodules

[Jens Lehmann]

Change the default behavior of "git checkout-index" to check out submodules
too. This can be prevented by using the new "--ignore-submodules" option.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/git-checkout-index.txt |    9 ++++++++-
 builtin/checkout-index.c             |    6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt
index d6aa6e1..dbd6625 100644
--- a/Documentation/git-checkout-index.txt
+++ b/Documentation/git-checkout-index.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
 		   [--stage=<number>|all]
-		   [--temp]
+		   [--temp] [--ignore-submodules]
 		   [-z] [--stdin]
 		   [--] [<file>]\*

@@ -68,6 +68,13 @@ OPTIONS
 	Only meaningful with `--stdin`; paths are separated with
 	NUL character instead of LF.

+--ignore-submodules::
+	Since version 1.8.0 the default behavior is to checkout populated
+	submodules recursively. When this option is used, the work trees of
+	submodules will not be updated, only the hash recorded in the
+	superproject will be changed (this was the default behavior until
+	1.8.0).
+
 \--::
 	Do not interpret any more arguments as options.

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index c35bad3..1e16d0d 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -213,7 +213,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
-	int force = 0, quiet = 0, not_new = 0;
+	int force = 0, quiet = 0, not_new = 0, ignore_submodules = 0;
 	struct option builtin_checkout_index_options[] = {
 		OPT_BOOLEAN('a', "all", &all,
 			"checks out all files in the index"),
@@ -238,6 +238,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK(0, "stage", NULL, NULL,
 			"copy out the files from named stage",
 			option_parse_stage),
+		OPT_BOOLEAN(0, "ignore-submodules", &ignore_submodules,
+			"don't update submodule work trees"),
 		OPT_END()
 	};

@@ -254,7 +256,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	state.force = force;
 	state.quiet = quiet;
 	state.not_new = not_new;
-	state.ignore_submodules = 1;
+	state.ignore_submodules = ignore_submodules;

 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
-- 
1.7.1.rc0.248.g09203

Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> +int checkout_submodule(const char *path, const unsigned char sha1[20], int force)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct child_process cp;
> +	const char *hex_sha1 = sha1_to_hex(sha1);
> +	const char *argv[] = {
> +		"checkout",
> +                force ? "-qf" : "-q",
> +		hex_sha1,
> +		NULL,
> +	};

Why force -q?

> +	strbuf_addf(&buf, "%s/.git/", path);
> +	if (!is_directory(buf.buf)) {
> +		strbuf_release(&buf);
> +		/* The submodule is not populated, so we can't check it out */
> +		return 0;
> +	}

This would give you an incorrect result if .git is a file that records
"gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
it would become a fairly important ingredient if we ever enhance the
submodule support to add submodule that disappears/reappears in the
history.

Re: [WIP PATCH 4/4] Teach checkout-index to recursively checkout submodules

[Junio C Hamano]

As a plumbing I would prefer to leave checkout-index as is; script writers
can choose to recurse if they choose to.

This is not limited to checkout-index, but what is the stance of this new
feature on failures from sub-checkout when there are local modifications
in the work tree?  Some parts of the work tree is checked out while the
ones after the failure that sorts later in the alphabetical order will not
be checked out, resulting in an inconsistent state (I am not saying that
it is good or bad---I am trying to see what the users should expect from
this new feature)?

Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Jens Lehmann]

Am 09.04.2010 23:59, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> +int checkout_submodule(const char *path, const unsigned char sha1[20], int force)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	struct child_process cp;
>> +	const char *hex_sha1 = sha1_to_hex(sha1);
>> +	const char *argv[] = {
>> +		"checkout",
>> +                force ? "-qf" : "-q",
>> +		hex_sha1,
>> +		NULL,
>> +	};
> 
> Why force -q?

Good question. I seem to have tried to silence the output of checkout
from run_command(), which AFIACS doesn't reach the console anyway
unless i want it to ... (while at the same time managing to mess up
the tabs in that line ... :-/ )


>> +	strbuf_addf(&buf, "%s/.git/", path);
>> +	if (!is_directory(buf.buf)) {
>> +		strbuf_release(&buf);
>> +		/* The submodule is not populated, so we can't check it out */
>> +		return 0;
>> +	}
> 
> This would give you an incorrect result if .git is a file that records
> "gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
> it would become a fairly important ingredient if we ever enhance the
> submodule support to add submodule that disappears/reappears in the
> history.

Right. This assumption is also present in add_submodule_odb() (used by
show_submodule_summary()) and is_submodule_modified(), so i just reused
it. This should be addressed in another patch.

Re: [WIP PATCH 4/4] Teach checkout-index to recursively checkout submodules

[Jens Lehmann]

Am 10.04.2010 00:04, schrieb Junio C Hamano:
> As a plumbing I would prefer to leave checkout-index as is; script writers
> can choose to recurse if they choose to.

O.k.; what about adding an option "--recurse-submodules" to activate that
behavior - without having to code it every time - when wanted?


> This is not limited to checkout-index, but what is the stance of this new
> feature on failures from sub-checkout when there are local modifications
> in the work tree?  Some parts of the work tree is checked out while the
> ones after the failure that sorts later in the alphabetical order will not
> be checked out, resulting in an inconsistent state (I am not saying that
> it is good or bad---I am trying to see what the users should expect from
> this new feature)?

I would like to see the same attitude that checkout has on local files:
Don't overwrite anything changed (unless forced to). And an inconsistent
state should never happen, the checks on the submodules should be done
before the first file or submodule is checked out (But i still have to
test and verify that behavior in yet to be added tests).

Re: [WIP PATCH 0/4] Recursively checkout submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Having found some Git time recently i started to tackle the next
> major issue on my agenda to improve submodule experience: Check
> out the matching versions of populated submodules when doing a
> checkout in the superproject (without having to do an explicit
> "git submodule update").
> ...
> Opinions?

Forgot to say the most important thing.  I like the general direction.

Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Jens Lehmann]

Am 10.04.2010 01:11, schrieb Jens Lehmann:
> Am 09.04.2010 23:59, schrieb Junio C Hamano:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>> +	strbuf_addf(&buf, "%s/.git/", path);
>>> +	if (!is_directory(buf.buf)) {
>>> +		strbuf_release(&buf);
>>> +		/* The submodule is not populated, so we can't check it out */
>>> +		return 0;
>>> +	}
>>
>> This would give you an incorrect result if .git is a file that records
>> "gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
>> it would become a fairly important ingredient if we ever enhance the
>> submodule support to add submodule that disappears/reappears in the
>> history.
> 
> Right. This assumption is also present in add_submodule_odb() (used by
> show_submodule_summary()) and is_submodule_modified(), so i just reused
> it. This should be addressed in another patch.

What about this one:

-----------------8<---------------
Subject: [PATCH] Teach diff --submodule and status to handle .git files in submodules

The simple test for an existing .git directory gives an incorrect result
if .git is a file that records "gitdir: overthere". So for submodules that
use a .git file, "git status" and the diff family - when the "--submodule"
option is given - did assume the submodule was not populated at all when
a .git file was used, thus generating wrong output or no output at all.

This is fixed by using read_gitfile_gently() to get the correct location
of the .git directory. While at it, is_submodule_modified() was cleaned up
to use the "dir" member of "struct child_process" instead of setting the
GIT_WORK_TREE and GIT_DIR environment variables.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 submodule.c                 |   33 ++++++++++++++++-----------------
 t/t4041-diff-submodule.sh   |   15 +++++++++++++++
 t/t7506-status-submodule.sh |   16 ++++++++++++++++
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/submodule.c b/submodule.c
index b3b8bc1..676d48f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -12,8 +12,15 @@ static int add_submodule_odb(const char *path)
 	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
 	int ret = 0;
+	const char *git_dir;

-	strbuf_addf(&objects_directory, "%s/.git/objects/", path);
+	strbuf_addf(&objects_directory, "%s/.git", path);
+	git_dir = read_gitfile_gently(objects_directory.buf);
+	if (git_dir) {
+		strbuf_reset(&objects_directory);
+		strbuf_addstr(&objects_directory, git_dir);
+	}
+	strbuf_addstr(&objects_directory, "/objects/");
 	if (!is_directory(objects_directory.buf)) {
 		ret = -1;
 		goto done;
@@ -132,7 +139,6 @@ void show_submodule_summary(FILE *f, const char *path,

 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	int i;
 	ssize_t len;
 	struct child_process cp;
 	const char *argv[] = {
@@ -141,16 +147,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		NULL,
 		NULL,
 	};
-	const char *env[LOCAL_REPO_ENV_SIZE + 3];
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *line, *next_line;
+	const char *git_dir;

-	for (i = 0; i < LOCAL_REPO_ENV_SIZE; i++)
-		env[i] = local_repo_env[i];
-
-	strbuf_addf(&buf, "%s/.git/", path);
-	if (!is_directory(buf.buf)) {
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile_gently(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+	if (!is_directory(git_dir)) {
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
@@ -158,21 +164,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);

-	strbuf_addf(&buf, "GIT_WORK_TREE=%s", path);
-	env[i++] = strbuf_detach(&buf, NULL);
-	strbuf_addf(&buf, "GIT_DIR=%s/.git", path);
-	env[i++] = strbuf_detach(&buf, NULL);
-	env[i] = NULL;
-
 	if (ignore_untracked)
 		argv[2] = "-uno";

 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
-	cp.env = env;
+	cp.env = local_repo_env;
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
+	cp.dir = path;
 	if (start_command(&cp))
 		die("Could not run git status --porcelain");

@@ -201,8 +202,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (finish_command(&cp))
 		die("git status --porcelain failed");

-	for (i = LOCAL_REPO_ENV_SIZE; env[i]; i++)
-		free((char *)env[i]);
 	strbuf_release(&buf);
 	return dirty_submodule;
 }
diff --git a/t/t4041-diff-submodule.sh b/t/t4041-diff-submodule.sh
index 11b1997..019acb9 100755
--- a/t/t4041-diff-submodule.sh
+++ b/t/t4041-diff-submodule.sh
@@ -329,4 +329,19 @@ index 0000000..$head7
 EOF
 "

+test_expect_success 'setup .git file for sm2' '
+	(cd sm2 &&
+	 REAL="$(pwd)/../.real" &&
+	 mv .git "$REAL"
+	 echo "gitdir: $REAL" >.git)
+'
+
+test_expect_success 'diff --submodule with .git file' '
+	git diff --submodule HEAD^ >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6...0000000 (submodule deleted)
+Submodule sm2 0000000...$head7 (new submodule)
+EOF
+'
+
 test_done
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index aeec1f6..3d4f85d 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -157,6 +157,22 @@ test_expect_success 'status with added and untracked file in modified submodule
 	EOF
 '

+test_expect_success 'setup .git file for sub' '
+	(cd sub &&
+	 rm -f new-file
+	 REAL="$(pwd)/../.real" &&
+	 mv .git "$REAL"
+	 echo "gitdir: $REAL" >.git) &&
+	 echo .real >>.gitignore &&
+	 git commit -m "added .real to .gitignore" .gitignore
+'
+
+test_expect_success 'status with added file in modified submodule with .git file' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status >output &&
+	grep "modified:   sub (new commits, modified content)" output
+'
+
 test_expect_success 'rm submodule contents' '
 	rm -rf sub/* sub/.git
 '
-- 
1.7.1.rc0.265.g16922.dirty

Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>>> This would give you an incorrect result if .git is a file that records
>>> "gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
>>> it would become a fairly important ingredient if we ever enhance the
>>> submodule support to add submodule that disappears/reappears in the
>>> history.
>> 
>> Right. This assumption is also present in add_submodule_odb() (used by
>> show_submodule_summary()) and is_submodule_modified(), so i just reused
>> it. This should be addressed in another patch.
>
> What about this one:

Looks sensible to me.  Thanks, and sorry for not catching this earlier.

Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

[Jens Lehmann]

Am 10.04.2010 20:44, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>>> This would give you an incorrect result if .git is a file that records
>>>> "gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
>>>> it would become a fairly important ingredient if we ever enhance the
>>>> submodule support to add submodule that disappears/reappears in the
>>>> history.
>>>
>>> Right. This assumption is also present in add_submodule_odb() (used by
>>> show_submodule_summary()) and is_submodule_modified(), so i just reused
>>> it. This should be addressed in another patch.
>>
>> What about this one:
> 
> Looks sensible to me.  Thanks, and sorry for not catching this earlier.

While glancing over add_submodule_odb(): It protects against adding
a /new/ odb twice, but assuming the submodules objects later live in
the odb of the superproject too it will happily add that to the
alt_odb_list the first time it sees it, no?
Not knowing the odb code, would it make sense to protect against that?
Or does adding the superprojects .git/objects to the alt_odb_list have
no negative consequences?