Teach fetch and pull to recursively fetch submodules

* Should the "--submodule-prefix" option - which is only used internally now - be a hidden option to "git fetch"?

But nonetheless I think this patch series is ok for inclusion as it does not change default behavior and gives people the opportunity to play with recursive fetch/pull by enabling one of the introduced config options.

Quoted 2 lines · Jens Lehmann

* Should the "--submodule-prefix" option - which is only used internally now - be a hidden option to "git fetch"?

Yes. (Any option that is useless outside scripts should be, imho.)

Quoted 3 lines

But nonetheless I think this patch series is ok for inclusion as it does not change default behavior and gives people the opportunity to play with recursive fetch/pull by enabling one of the introduced config options.

Except for the .gitmodules detail I mentioned, it looks good to me. Looking forward to trying it out.

Quoted 2 lines · Jens Lehmann

* Should the "--submodule-prefix" option - which is only used internally now - be a hidden option to "git fetch"?

Yes. (Any option that is useless outside scripts should be, imho.)

Ok, here is the updated version of patch 1/3 making "--submodule-prefix" a hidden option which is not documented in Documentation/fetch-options.txt .

t/t5526-fetch-submodules.sh new file +12 -0

@@ -0,0 +1,109 @@
+#!/bin/sh
+# Copyright (c) 2010, Jens Lehmann
+
+test_description='Recursive "git fetch" for submodules'
+
+. ./test-lib.sh
+
+pwd=$(pwd)
+
+add_upstream_commit() {
+	(
+		cd submodule &&

Quoted 2 lines · Jens Lehmann

Ok, here is the updated version of patch 1/3 making "--submodule-prefix" a hidden option which is not documented in Documentation/fetch-options.txt .

Oh, I missed that you were proposing leaving it undocumented. On one hand, it is nice when debugging if all options are documented; but on the other hand, there is precedent in --rebasing for hiding implementation details like this. Unlike -h output, the manual is not so space-constrainted, so...

Quoted 19 lines

t/t5526-fetch-submodules.sh new file +16 -0

@@ -0,0 +1,109 @@
+#!/bin/sh
+# Copyright (c) 2010, Jens Lehmann
+
+test_description='Recursive "git fetch" for submodules'
+
+. ./test-lib.sh
+
+pwd=$(pwd)
+
+add_upstream_commit() {
+	(
+		cd submodule &&
+		head1=$(git rev-parse --short HEAD) &&
+		echo new >> subfile &&
+		test_tick &&
+		git add subfile &&

t/t5526-fetch-submodules.sh line 22

Quoted 6 lines

t/t5526-fetch-submodules.sh new file +6 -0

+		git commit -m new subfile &&
+		head2=$(git rev-parse --short HEAD) &&
+		echo "From $pwd/submodule" > ../expect.err &&
+		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err
+	)
+	(

Missing &&?

Hope that helps. These are just tiny nitpicks; I am happy with the patch with or without them.

Documentation/fetch-options.txt +7 -0

index 594f7ce..a9cb8ab 100644
@@ -67,6 +67,13 @@ endif::git-pull[]
 --recurse-submodules::
 	Use this option to fetch new commits of all populated submodules too.
 
+ifndef::git-pull[]
+--submodule-prefix=<path>::
+	Prepend <path> to paths printed in informative messages
+	such as "Fetching submodule foo".  This option is used
+	internally when recursing over submodules.
+endif::git-pull[]
+
 -u::
 --update-head-ok::
 	By default 'git fetch' refuses to update the head which

t/t5526-fetch-submodules.sh +1 -1

index 107317a..9c1d3a0 100755
@@ -18,7 +18,7 @@ add_upstream_commit() {
 		head2=$(git rev-parse --short HEAD) &&
 		echo "From $pwd/submodule" > ../expect.err &&
 		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err
-	)
+	) &&
 	(
 		cd deepsubmodule &&
 		head1=$(git rev-parse --short HEAD) &&

Until now you had to call "git submodule update" (without -N|--no-fetch option) or something like "git submodule foreach git fetch" to fetch new commits in populated submodules from their remote.
...

builtin/fetch.c +0 -0

index d35f000..db3fba3 100644
...

@@ -784,28 +790,36 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 	return 1;
 }
 

submodule.c +7 -1

index 91a4758..4d9b774 100644
@@ -63,7 +63,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
-static int submodule_config(const char *var, const char *value, void *cb)
+int submodule_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);

@@ -229,6 +229,70 @@ void show_submodule_summary(FILE *f, const char *path,
 	strbuf_release(&sb);
 }
 
+int fetch_populated_submodules(int num_options, const char **options,
+			       const char *prefix, int quiet)
+{
+	int i, result = 0, argc = 0;
+	struct child_process cp;
+	const char **argv;

Quoted 13 lines · Jens Lehmann

Until now you had to call "git submodule update" (without -N|--no-fetch option) or something like "git submodule foreach git fetch" to fetch new commits in populated submodules from their remote.
...

builtin/fetch.c +0 -0

index d35f000..db3fba3 100644
...

@@ -784,28 +790,36 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 	return 1;
 }
 

builtin/fetch.c static int add_remote_or_group(const char *name, struct string_list *list) · line 819

Quoted 10 lines

builtin/fetch.c +5 -4

-static int fetch_multiple(struct string_list *list)
+static void add_options_to_argv(int *argc, const char **argv)
 {
-	int i, result = 0;
-	const char *argv[11] = { "fetch", "--append" };
-...
+static int fetch_multiple(struct string_list *list)
+{
+	int i, result = 0;
+	const char *argv[12] = { "fetch", "--append" };

This used to be 11; are we adding something new? Ahh, possibly "--recurse_submodules".

Quoted 28 lines

submodule.c +12 -1

index 91a4758..4d9b774 100644
@@ -63,7 +63,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
-static int submodule_config(const char *var, const char *value, void *cb)
+int submodule_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);

@@ -229,6 +229,70 @@ void show_submodule_summary(FILE *f, const char *path,
 	strbuf_release(&sb);
 }
 
+int fetch_populated_submodules(int num_options, const char **options,
+			       const char *prefix, int quiet)
+{
+	int i, result = 0, argc = 0;
+	struct child_process cp;
+	const char **argv;
+	struct string_list_item *name_for_path;
+	const char *work_tree = get_git_work_tree();
+	if (!work_tree)
+		return 0;
+

submodule.c void show_submodule_summary(FILE *f, const char *path, · line 247

Quoted 5 lines

submodule.c +5 -0

+	if (!the_index.initialized)
+		if (read_cache() < 0)
+			die("index file corrupt");
+
+	argv = xcalloc(num_options + 5, sizeof(const char *));

Where is this '5' coming from? "fetch" "--submodule-prefix", the prefix, and the terminating NULL? What did I miss?

Am 09.12.2010 22:16, schrieb Junio C Hamano:
...

Quoted 11 lines

Quoted 11 lines

(quoted diff) +11 -0

+int fetch_populated_submodules(int num_options, const char **options,
+			       const char *prefix, int quiet)
+{
+	int i, result = 0, argc = 0;
+	struct child_process cp;
+	const char **argv;
+	struct string_list_item *name_for_path;
+	const char *work_tree = get_git_work_tree();
+	if (!work_tree)
+		return 0;
+

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (!the_index.initialized)
+		if (read_cache() < 0)
+			die("index file corrupt");
+
+	argv = xcalloc(num_options + 5, sizeof(const char *));

Where is this '5' coming from? "fetch" "--submodule-prefix", the prefix, and the terminating NULL? What did I miss?

No, you didn't miss anything but I have been off by one ... '4' is sufficient here.

Quoted 2 lines · Jens Lehmann

No, you didn't miss anything but I have been off by one ... '4' is sufficient here.

Ok, thanks for double checking.

-- >8 --
Subject: [PATCH] fetch_populated_submodules(): document dynamic allocation size

t/t5526-fetch-submodules.sh +0 -0

@@ -106,4 +106,40 @@ test_expect_success "--dry-run propagates to submodules" '
 	test_cmp expect.err actual.err
 '
 

Quoted 6 lines · Jens Lehmann

t/t5526-fetch-submodules.sh +0 -0

@@ -106,4 +106,40 @@ test_expect_success "--dry-run propagates to submodules" '
 	test_cmp expect.err actual.err
 '
 

t/t5526-fetch-submodules.sh test_expect_success "--dry-run propagates to submodules" ' · line 118

Quoted 10 lines

t/t5526-fetch-submodules.sh +10 -0

+test_expect_success "recurseSubmodules=true propagates into submodules" '
+	add_upstream_commit &&
+	(
+		cd downstream &&
+		git config fetch.recurseSubmodules true
+		git fetch >../actual.out 2>../actual.err
+	) &&
+	test_cmp expect.out actual.out &&
+	test_cmp expect.err actual.err
+'

This configuration item is read from .gitmodules, too, right? Would it be easy (or desirable) to make it not read from there? Either way, it would be nice to have a test so the behavior doesn't change without anyone noticing.

Am 11.11.2010 01:02, schrieb Jonathan Nieder:

Quoted 4 lines

This configuration item is read from .gitmodules, too, right? Would it be easy (or desirable) to make it not read from there? Either way, it would be nice to have a test so the behavior doesn't change without anyone noticing.

"fetch.recurseSubmodules" is only read from .git/config. The one read first from .gitmodules and then from .git/config is the per-submodule setting "submodule.<name>.fetchRecurseSubmodules" added in 3/3.

But
maybe I should add a test that "fetch.recurseSubmodules" also works when set in the global config ...

Quoted 3 lines

"fetch.recurseSubmodules" is only read from .git/config. The one read first from .gitmodules and then from .git/config is the per-submodule setting "submodule.<name>.fetchRecurseSubmodules" added in 3/3.

Sorry for the nonsense. Would it be easy (or desirable) to make _that_ one not be read from .gitmodules?

I think the motivation behind having a way to read it from .gitmodules is so that project can suggest the default for convenience (e.g. "almost everybody who interacts with this project wants these submodules checked out and kept updated").

Quoted 4 lines · Junio C Hamano

I think the motivation behind having a way to read it from .gitmodules is so that project can suggest the default for convenience (e.g. "almost everybody who interacts with this project wants these submodules checked out and kept updated").

Yes, that makes some sense to me. Except wouldn't it be a single configuration item? "These submodules should be checked out in all but unusual situations, so check them out automatically and keep them updated."

Maybe a person setting this to false actually means "This submodule has its url set to a repository that is updated very frequently, and most updates are not relevant to the superproject." Unfortunately, I think the result would be a poor user experience: when an update comes that _is_ important to the superproject, what happens?

 $ git fetch
 ... go on plane ...
 $ git merge @{u} && git submodule update --no-fetch --recursive
 [...]
 fatal: reference is not a tree: f1c596a3895643d0969a15b8e945bf0c0072e470

Hmm. I think in that scenario a better solution would be to point the submodule url point to a project-specific clone that is updated less frequently.

What am I missing?

Am 11.11.2010 20:00, schrieb Jonathan Nieder:

Quoted 4 lines

Yes, that makes some sense to me. Except wouldn't it be a single configuration item? "These submodules should be checked out in all but unusual situations, so check them out automatically and keep them updated."

Hmm, but we have at least three modes of how to update them:

1) Never fetch the submodule (to get new commits the user has to run "git fetch --recurse-submodules" by hand)

2) Fetch the submodule each time you fetch the superproject (Which is really handy when you do development in the submodule too but can be really inconvenient when you don't)

3) Update submodules only when new recorded commits are fetched in the superproject (This mode is not added with the current patch series but will be in one of the next)

So you would need a config option for that anyway, no? And that is why I'd rather like to have a separate fetch option to control that behavior instead of an implicit "if-it's-to-be-checked-out-fetch-it-too" approach.

Quoted 20 lines · Jens Lehmann

Am 11.11.2010 20:00, schrieb Jonathan Nieder:

Quoted 4 lines

Yes, that makes some sense to me. Except wouldn't it be a single configuration item? "These submodules should be checked out in all but unusual situations, so check them out automatically and keep them updated."

Hmm, but we have at least three modes of how to update them:

1) Never fetch the submodule (to get new commits the user has to run "git fetch --recurse-submodules" by hand)

2) Fetch the submodule each time you fetch the superproject (Which is really handy when you do development in the submodule too but can be really inconvenient when you don't)

3) Update submodules only when new recorded commits are fetched in the superproject (This mode is not added with the current patch series but will be in one of the next)

Quoted 3 lines

So you would need a config option for that anyway, no? And that is why I'd rather like to have a separate fetch option to control that behavior instead of an implicit "if-it's-to-be-checked-out-fetch-it-too" approach.

I still think I am missing something.

Traditionally, git has been a _content_ tracker. The configuration that gets transmitted (.gitignore, .gitattributes, .gitmodules) would only represent basic information needed for that content to remain usable and sensible.

In the case of .gitmodules, it seems that two concepts are being conflated:

A. Configuration based on the user's preferences. Absolutely, a person
deserves to be able to easily choose between (1), (2), and (3) as described above.

B. Metadata about the content that should be shared. For example, "this
submodule would be checked out in all but unusual circumstances" is a useful thing to be able to declare.

Probably I am missing something big, but fetchsubmodules as currently defined seem like something in category A and not in category B. Partially because if we ever implement option (3), that is what almost _every_ casual consumer will want. So why should they be stuck with these configuration files specifying (1) and (2) when they check out old revisions?

All that said, I do not think it is unreasonable in some situations for a project to want to share configuration of type (A) between members of a project; for example, lots of projects share hooks and that's great. I just don't think git should set it up automatically --- better to require an explicit user action like "sh useprojectconfiguration.sh" (an action more explicit than "please initialize and checkout all relevant submodules") for such cases.

No, I think these concepts aren't conflated at all:

- Category A is handled by .git/config

- Category B is handled by the .gitmodules file

There are people putting lots of large files in submodules for better performance and they almost always never want to fetch (or even stat) them, so (1) is for them and it's cool that their upstream can configure that, avoiding to have every developer to repeat their "obvious" choice after each clone again (and that might only be needed for some submodules, so a repo-wide config doesn't necessarily help them).

And when you are on a superproject branch actively developing inside a submodule, you may want to increase fetch-activity to fetch all new commits in the submodule even if they aren't referenced in the superproject (yet), as that might be just what your fellow developers are about to do. And the person setting up that branch could do that once for all users so they don't have to repeat it in every clone.

And
when switching away from that branch all those developers cannot forget to reconfigure to fetch-on-demand, so not having that in .git/config is a plus here too.

You have no other choice for hooks because of security concerns. But I can't see any downsides in leaving upstream *the choice* to configure default submodule behavior. Lots of people - including me - want that for clone and checkout.

Quoted 6 lines · Jens Lehmann

There are people putting lots of large files in submodules for better performance and they almost always never want to fetch (or even stat) them, so (1) is for them and it's cool that their upstream can configure that, avoiding to have every developer to repeat their "obvious" choice after each clone again (and that might only be needed for some submodules, so a repo-wide config doesn't necessarily help them).

Wouldn't (3) work for these people, too?

I think we are getting closer to an explanation. I can look into adding documentation for this on top when finished.

Quoted 6 lines

And when you are on a superproject branch actively developing inside a submodule, you may want to increase fetch-activity to fetch all new commits in the submodule even if they aren't referenced in the superproject (yet), as that might be just what your fellow developers are about to do. And the person setting up that branch could do that once for all users so they don't have to repeat it in every clone.

This one seems less reasonable to me. It seems like a way to remotely help developers get a nice setup, rather than a declaration about the content.

Let me take an unrelated example to illustrate what I mean. Some projects might want all their developers to use branch.autosetuprebase, to avoid confusion since the update hook is going to reject mergy history anyway. That seems like a perfectly reasonable desire to me, and I'd encourage them to add a script that sets everything up following the policies of their project.

Now git could even learn to read a .gitconfig file including settings like that one that do not have a security impact. It would make lots of people happy, and individuals could override settings they really dislike in ~/.gitconfig. Should we do it?

I think no, for reasons of intuitiveness and predictability.

On the other hand, scenarios like (1) might mean we have to support such things despite that downside.

Quoted 4 lines

And
when switching away from that branch all those developers cannot forget to reconfigure to fetch-on-demand, so not having that in .git/config is a plus here too.

Yes, the "read .gitmodules first and then .git/config" is a very nice enhancement --- thanks!

Quoted 4 lines

You have no other choice for hooks because of security concerns. But I can't see any downsides in leaving upstream *the choice* to configure default submodule behavior. Lots of people - including me - want that for clone and checkout.

There is one setting that it is obvious to me for upstream should be able to set:

"these submodules are a necessary part of the project;
always (at clone time, fetch time, checkout, etc) make sure they are available"

I could easily be convinced about others, but there ought to be a use case to outweigh the "subtle behavior changing behind my back" syndrome.

And again: thanks for doing all this work. It's inspiring. (Next step recursive push?)

Quoted 4 lines · Jonathan Nieder

Quoted 3 lines

"fetch.recurseSubmodules" is only read from .git/config. The one read first from .gitmodules and then from .git/config is the per-submodule setting "submodule.<name>.fetchRecurseSubmodules" added in 3/3.

Quoted 2 lines

Sorry for the nonsense. Would it be easy (or desirable) to make _that_ one not be read from .gitmodules?

I think the motivation behind having a way to read it from .gitmodules is so that project can suggest the default for convenience (e.g. "almost everybody who interacts with this project wants these submodules checked out and kept updated").

Traditionally the suggestions kept in .gitmodules were propagated to the config when the submodule was initialized, and at runtime we read only from the config from then on without reading from .gitmodules, so that once the user decides to follow what the project suggests (or customize that away), the preference would stick to the repository.

That arrangement does not cater well to people who want to follow along whatever the project's suggestion of the day, so we might want to change things so that we if we find it in the config, we stop there and use what we found, otherwise we use what is in the in-tree gitmodules; I suspect we might require some changes to "submodule init" not to copy certain things to the config for that to work, though...