Teach fetch/pull the on-demand mode and make it the default

*) The fetch is only done when the recorded submodule commit isn't
already present.

I tend to think that this is suited for 1.7.5 but don't have any objections against holding it back until 1.8.0 either. What do others think?

Quoted 2 lines · Jens Lehmann

*) The fetch is only done when the recorded submodule commit isn't
already present.

I like this part a lot.

Quoted 3 lines

I tend to think that this is suited for 1.7.5 but don't have any objections against holding it back until 1.8.0 either. What do others think?

I see no backward-compatibility to wait for this, but I would be more included to trust people using "git submodule update" heavily than I do.

The "submodule update" change could cause the following to break. Would that be disruptive?

	cd submodule
	git fetch --no-recurse-submodules
	...

	cd ..
	bin/script-to-update-submodules-that-calls-submodule-update

To be able to access all commits of populated submodules referenced by the superproject it is sufficient to only then let "git fetch" recurse into a submodule when the new commits fetched in the superproject record new commits for it.

There is currently no infrastructure for storing deleted and new submodules in the .git directory of the superproject. Thats why fetch and pull for now only fetch submodules that are already checked out and are not renamed.

Documentation/fetch-options.txt +0 -0

@@ -73,6 +73,14 @@ ifndef::git-pull[]
 	Prepend <path> to paths printed in informative messages

Documentation/fetch-options.txt +1 -0

+++ b/submodule.c

Documentation/fetch-options.txt +0 -0

@@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)

+static void submodule_collect_changed_cb(struct diff_queue_struct *q,

Documentation/fetch-options.txt +1 -0

+	while ((commit = get_revision(&rev))) {

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

Documentation/fetch-options.txt +0 -0

@@ -192,4 +192,113 @@ test_expect_success "--no-recurse-submodules overrides config setting" '

Quoted 4 lines · Jens Lehmann

To be able to access all commits of populated submodules referenced by the superproject it is sufficient to only then let "git fetch" recurse into a submodule when the new commits fetched in the superproject record new commits for it.

Exciting stuff. This would use the default refspec rather than fetching the bare minimum of commits in submodules, right?

Quoted 4 lines

Documentation/fetch-options.txt +0 -0

@@ -73,6 +73,14 @@ ifndef::git-pull[]
 	Prepend <path> to paths printed in informative messages

Documentation/fetch-options.txt ifndef::git-pull[] · line 83

Quoted 10 lines

Documentation/fetch-options.txt +8 -0

 	such as "Fetching submodule foo".  This option is used
 	internally when recursing over submodules.
+
+--submodule-default=[yes|on-demand]::
+	This option is used internally to set the submodule recursion default
+	to either a boolean configuration value representing "true" (for
+	unconditonal recursion) or to "on-demand" (when only those submodules
+	should be fetched of which new commits have been fetched in its
+	superproject).
+	Using the "--[no-]recurse-submodules" option ignores this setting.

Could this be combined with the --recurse-submodules, with a "last instance of the option wins" rule? Something like:

--recurse-submodules[=(yes | no | changed)]::
--no-recurse-submodules::

Quoted 1 line

Documentation/fetch-options.txt +1 -0

+++ b/submodule.c

[...]

Quoted 1 line

Documentation/fetch-options.txt +0 -0

@@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)

[...]

Quoted 1 line

+static void submodule_collect_changed_cb(struct diff_queue_struct *q,

[...]

Documentation/fetch-options.txt

Quoted 5 lines

Documentation/fetch-options.txt +5 -0

+		if (S_ISGITLINK(p->one->mode)) {
+			/* NEEDSWORK: We should honor the name configured in
+			 * the .gitmodules file of the commit we are examining
+			 * here to be able to correctly follow submodules
+			 * being moved around. */

Hmm, a sort of variant on rename detection. Does "git submodule update" / "git checkout --recurse-submodules" use .gitmodules to move pre-populated subrepositories?

Documentation/fetch-options.txt

Quoted 7 lines

Documentation/fetch-options.txt +7 -0

+			/* Submodule is new or was moved here */
+			/* NEEDSWORK: When the .git directories of submodules
+			 * live inside the superprojects .git directory some
+			 * day we should fetch new submodules directly into
+			 * that location too when config or options request
+			 * that so they can be checked out from there. */
+			continue;

Maybe this can be mentioned in a BUGS section on the git-fetch(1) manpage to give readers a warning and clue about the intended meaning of --recurse-submodules?

I'm afraid a certain kind of person might get used to the "don't fetch new submodules" behavior (e.g., if upstream is including unneeded convenience copies of libraries right and left in addition to some useful submodules).

Documentation/fetch-options.txt

Quoted 6 lines

Documentation/fetch-options.txt +6 -0

+void check_for_new_submodule_commits(unsigned char new_sha1[20])
+{
+	struct rev_info rev;
+	struct commit *commit;
+	int argc = 5;
+	const char *argv[] = {NULL, NULL, "--not", "--branches", "--remotes", NULL};

Nit: maybe

	const char *argv[] = ...;
	int argc = ARRAY_SIZE(argv) - 1;

?

Documentation/fetch-options.txt

Quoted 3 lines

Documentation/fetch-options.txt +3 -0

+
+	init_revisions(&rev, NULL);
+	argv[1] = xstrdup(sha1_to_hex(new_sha1));

Maybe:

	char *new_rev;
	...
	argv[1] = new_rev = xstrdup(...);
	...
	free(new_rev);

Documentation/fetch-options.txt

Quoted 4 lines

Documentation/fetch-options.txt +4 -0

+	setup_revisions(argc, argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+

Maybe a comment so the reader doesn't have to delve deeper?

	/*
	 * Collect checked out submodules that have changed upstream
	 * in "changed_submodule_paths".
	 */

Quoted 1 line

Documentation/fetch-options.txt +1 -0

+	while ((commit = get_revision(&rev))) {

[...]

Quoted 2 lines

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

Documentation/fetch-options.txt +0 -0

@@ -192,4 +192,113 @@ test_expect_success "--no-recurse-submodules overrides config setting" '

[...]

Documentation/fetch-options.txt

Quoted 3 lines

Documentation/fetch-options.txt +3 -0

+	echo "Fetching submodule submodule" > expect.out.sub &&
+	echo "From $pwd/." > expect.err.sub &&
+	echo "   $head1..$head2  master     -> origin/master" >> expect.err.sub

I wonder if we should be testing the output in this much detail.

Wouldn't it be nicer to check the remote-tracking refs to make sure the lasting effects were correct, rather than the detailed output format?

So: aside from the option name, nothing but nits. Thanks; that was fun.

Am 24.02.2011 00:07, schrieb Jonathan Nieder:

Quoted 5 lines

Could this be combined with the --recurse-submodules, with a "last instance of the option wins" rule? Something like:

--recurse-submodules[=(yes | no | changed)]::
--no-recurse-submodules::

Nope, as this only sets the default. "--recurse-submodules" overrides anything configured, which is not what we want here. This option should only set the default.

Quoted 11 lines

submodule.c void set_config_fetch_recurse_submodules(int value) · line 292

Quoted 7 lines

submodule.c +7 -0

+			/* Submodule is new or was moved here */
+			/* NEEDSWORK: When the .git directories of submodules
+			 * live inside the superprojects .git directory some
+			 * day we should fetch new submodules directly into
+			 * that location too when config or options request
+			 * that so they can be checked out from there. */
+			continue;

Maybe this can be mentioned in a BUGS section on the git-fetch(1) manpage to give readers a warning and clue about the intended meaning of --recurse-submodules?

I'm not sure I understand what you mean by this, right now this can only work for populated submodules. I hope this will change soon, but I'm not quite there yet ;-)

Quoted 7 lines

Maybe:

	char *new_rev;
	...
	argv[1] = new_rev = xstrdup(...);
	...
	free(new_rev);

I'm not sure I get what the extra variable gains us ...

Quoted 1 line

Maybe a comment so the reader doesn't have to delve deeper?

?

Quoted 6 lines

	/*
	 * Collect checked out submodules that have changed upstream
	 * in "changed_submodule_paths".
	 */

Quoted 1 line

submodule.c +1 -0

+	while ((commit = get_revision(&rev))) {

Thanks!

submodule.c +9 -0

@@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)
...
+static void submodule_collect_changed_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
+{
+	int i;
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		if (!S_ISGITLINK(p->two->mode))
+			continue;

submodule.c void handle_ignore_submodules_arg(struct diff_options *diffopt, · line 158

Quoted 9 lines · Jens Lehmann

submodule.c +3 -0

index 6f1c107..c8c3a99 100644
@@ -152,6 +153,20 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
...
+int parse_fetch_recurse_submodules_arg(const char *arg)
+{
+	switch (git_config_maybe_bool("", arg)) {

It's a bit unusual to see "" as the variable name; doesn't config-maybe-bool barf when arg is not what it likes, with the name as part of its message?

Quoted 11 lines

submodule.c +9 -0

@@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)
...
+static void submodule_collect_changed_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
+{
+	int i;
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		if (!S_ISGITLINK(p->two->mode))
+			continue;

submodule.c void set_config_fetch_recurse_submodules(int value) · line 284

Quoted 10 lines

submodule.c +10 -0

+
+		if (S_ISGITLINK(p->one->mode)) {
+			/* NEEDSWORK: We should honor the name configured in
+			 * the .gitmodules file of the commit we are examining
+			 * here to be able to correctly follow submodules
+			 * being moved around. */
+			struct string_list_item *path;
+			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
+			if (!path)
+				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));

I wondered why there is no mention of "data" in the implementation of this function; changed_submodule_paths global is used instead here.

I do not see anywhere the global string_list is initialized, freed, nor re-initialized for reuse. Are you guaranteeing that the caller only calls the check_for_new_submodule_commits() function once, and if so how? The function is called from update_local_ref() in builtin/fetch.c, which in turn gets called number of times during a fetch. IOW, does the code do the right thing when you are fetching more than one refs?

But maybe
fetch_populated_submodules() should empty the string_list it just worked through?

As fetch and pull now by default just fetch those submodules
for which new commits have been fetched in the superproject, a command line option to enforce that behavior is needed to be able to override configuration settings.

Quoted 4 lines · Jens Lehmann

As fetch and pull now by default just fetch those submodules
for which new commits have been fetched in the superproject, a command line option to enforce that behavior is needed to be able to override configuration settings.

Probably this should go first in the series (the usual procedure:
first command line for easy testing, then configuration for routine use, then defaults).

submodule.c int parse_submodule_config_option(const char *var, const char *value) · line 116

Quoted 10 lines · Jens Lehmann

submodule.c +1 -1

index cccd728..b477c3c 100644
@@ -113,7 +113,7 @@ int parse_submodule_config_option(const char *var, const char *value)
 		if (!config)
 			config = string_list_append(&config_fetch_recurse_submodules_for_name,
 						    strbuf_detach(&submodname, NULL));
-		config->util = git_config_bool(var, value) ? (void *)1 : NULL;
+		config->util = (void *)(size_t)parse_fetch_recurse_submodules_arg(value);

What is this double-cast about?

submodule.c int fetch_populated_submodules(int num_options, const char **options, · line 381

Quoted 5 lines

submodule.c +2 -0

@@ -376,8 +376,13 @@ int fetch_populated_submodules(int num_options, const char **options,
...
+				if ((size_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_OFF)
 					continue;
+				if ((size_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_ON_DEMAND) {

Likewise here; size_t feels a strange type to cast to in this comparison between (void *) and an enum, no?

submodule.c +0 -0

index b477c3c..ddb0a29 100644

Quoted 8 lines · Jens Lehmann

submodule.c +0 -0

index b477c3c..ddb0a29 100644
@@ -263,6 +263,13 @@ void set_config_fetch_recurse_submodules(int value)
 	config_fetch_recurse_submodules = value;
 }
 

submodule.c void set_config_fetch_recurse_submodules(int value) · line 271

Quoted 6 lines

submodule.c +6 -0

+static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
+{
+	if (!add_submodule_odb(path))
+		return lookup_commit_reference(sha1) != 0;
+	return 0;
+}

Don't you need to prove the usual "reachabile from the refs" here, instead of just the presense of a commit object?

Am 24.02.2011 00:21, schrieb Junio C Hamano:

Quoted 10 lines

Quoted 8 lines · Jens Lehmann

submodule.c +0 -0

index b477c3c..ddb0a29 100644
@@ -263,6 +263,13 @@ void set_config_fetch_recurse_submodules(int value)
 	config_fetch_recurse_submodules = value;
 }
 

Quoted 9 lines

submodule.c void set_config_fetch_recurse_submodules(int value) · line 271

Quoted 6 lines

submodule.c +6 -0

+static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
+{
+	if (!add_submodule_odb(path))
+		return lookup_commit_reference(sha1) != 0;
+	return 0;
+}

Don't you need to prove the usual "reachabile from the refs" here, instead of just the presense of a commit object?

Looks like I would. Could you please enlighten me?