submodule pushes be extra careful

PATCH In review Stefan Beller 3 participants 10 messages Oct 6, 2016 – Oct 11, 2016

Share & tools

Stefan Beller

author

2016-10-06 19:37 UTC #0 lore raw

cover submodule pushes be extra careful

This is a reroll of
http://public-inbox.org/git/[email protected]/
([PATCHv3 1/2] push: change submodule default to check when submodules exist) but with a test.

As we only have a heuristic, the test failed initially as these tests don't have any configuration at all nor do they have the submodule repos in the superprojects git dir. So I was looking for a cheap way to add a config.

I could have written the config myself via git config, I think it is more idiomatic to use a submodule command for that. However just getting the configuration added is not possible with a submodule command, so I made one in the first patch.

Thanks,
Stefan

Stefan Beller (2):
submodule add: extend force flag to add existing repos
push: change submodule default to check when submodules exist

 builtin/push.c                 | 15 ++++++++++++++-
 git-submodule.sh               | 10 ++++++++--
 t/t5531-deep-submodule-push.sh |  6 +++++-
 t/t7400-submodule-basic.sh     | 14 ++++++++++++++
 4 files changed, 41 insertions(+), 4 deletions(-)

Signature · 1 line

2.10.1.353.g1629400

Stefan Beller

author

2016-10-06 19:37 UTC #1 lore raw

PATCH 1/2 submodule add: extend force flag to add existing repos

Currently the force flag in git submodule add takes care of possibly ignored files or when a name collision occurs.

However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was done (i.e. no .gitmodules file nor any entry in .git/config) and you want to generate these config entries. For this situation allow git submodule add to proceed if there is already a submodule at the given path in the index.

Signed-off-by Stefan Beller <[email protected]>

---
 git-submodule.sh           | 10 ++++++++--
 t/t7400-submodule-basic.sh | 14 ++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

git-submodule.sh +8 -2

index a024a13..3762616 100755
@@ -207,8 +207,14 @@ cmd_add()
 			tstart
 			s|/*$||
 		')
-	git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
-	die "$(eval_gettext "'\$sm_path' already exists in the index")"
+	if test -z "$force"
+	then
+		git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
+		die "$(eval_gettext "'\$sm_path' already exists in the index")"
+	else
+		git ls-files -s "$sm_path" | sane_grep -v "^160000" > /dev/null 2>&1 &&
+		die "$(eval_gettext "'\$sm_path' already exists in the index and is not a submodule")"
+	fi
 
 	if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
 	then

t/t7400-submodule-basic.sh +14 -0

index b77cce8..c09ce0d 100755
@@ -152,6 +152,20 @@ test_expect_success 'submodule add to .gitignored path with --force' '
 	)
 '
 
+test_expect_success 'submodule add to reconfigure existing submodule with --force' '
+	(
+		cd addtest-ignore &&
+		git submodule add --force bogus-url submod &&
+		git submodule add -b initial "$submodurl" submod-branch &&
+		test "bogus-url" = "$(git config -f .gitmodules submodule.submod.url)" &&
+		test "bogus-url" = "$(git config submodule.submod.url)" &&
+		# Restore the url
+		git submodule add --force "$submodurl" submod
+		test "$submodurl" = "$(git config -f .gitmodules submodule.submod.url)" &&
+		test "$submodurl" = "$(git config submodule.submod.url)"
+	)
+'
+
 test_expect_success 'submodule add --branch' '
 	echo "refs/heads/initial" >expect-head &&
 	cat <<-\EOF >expect-heads &&

Signature · 1 line

2.10.1.353.g1629400

Junio C Hamano

2016-10-06 20:11 UTC #2 lore raw

Re: [PATCH 1/2] submodule add: extend force flag to add existing repos

Quoted 10 lines · Stefan Beller

Currently the force flag in git submodule add takes care of possibly ignored files or when a name collision occurs.
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was done (i.e. no .gitmodules file nor any entry in .git/config) and you want to generate these config entries. For this situation allow git submodule add to proceed if there is already a submodule at the given path in the index.

Quoted 2 lines

Signed-off-by Stefan Beller <[email protected]>
---

Yup, the goal makes perfect sense.

I vaguely recall discussing this exact issue of "git submodule add" that refuses to add a path that already is a gitlink (via "git add" that has previously been run) elsewhere on this list some time ago.

Heiko Voigt

2016-10-07 12:52 UTC #3 lore raw

Re: [PATCH 1/2] submodule add: extend force flag to add existing repos

Quoted 11 lines · Junio C Hamano

Quoted 9 lines · Stefan Beller
Currently the force flag in git submodule add takes care of possibly ignored files or when a name collision occurs.
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was done (i.e. no .gitmodules file nor any entry in .git/config) and you want to generate these config entries. For this situation allow git submodule add to proceed if there is already a submodule at the given path in the index.

Is it important that the submodule is in the index? How about worktree? From the index entry alone we can not deduce the values anyway. So I would say the submodule has to be in the worktree, no matter what is in the index. If its not in the index we can also add it.

BTW, that is the way I imagined submodules would work in the first
place: just clone and add them, like I described here[1].

Quoted 8 lines

Quoted 2 lines
Signed-off-by Stefan Beller <[email protected]>
---
Yup, the goal makes perfect sense.
I vaguely recall discussing this exact issue of "git submodule add" that refuses to add a path that already is a gitlink (via "git add" that has previously been run) elsewhere on this list some time ago.

Yes there was a discussion, see the link.

Cheers Heiko

[1] http://public-inbox.org/git/%[email protected]%3E/

Stefan Beller

author

2016-10-07 17:26 UTC #4 lore raw

Re: [PATCH 1/2] submodule add: extend force flag to add existing repos

Quoted 6 lines · Heiko Voigt

Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Stefan Beller
Currently the force flag in git submodule add takes care of possibly ignored files or when a name collision occurs.

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was done (i.e. no .gitmodules file nor any entry in .git/config) and you want to generate these config entries. For this situation allow git submodule add to proceed if there is already a submodule at the given path in the index.
Is it important that the submodule is in the index?

If it is not in the index, it already works.

Quoted 2 lines

How about worktree?
From the index entry alone we can not deduce the values anyway.

Right, but as of now this is the only show stopper, i.e.
* you have an existing repo? -> fine, it works with --force
* you even ignored that repo -> --force knows how to do it.
* you already have a gitlink -> Sorry, you're out of luck.

So that is why I stressed index in this commit message, as it is only about this case.

Quoted 2 lines

[1] http://public-inbox.org/git/%[email protected]%3E/

Current situation:

Quoted 3 lines

clone the submodule into a directory
git submodule add --force
git commit everything

works fine, but:

Quoted 6 lines

clone the submodule into a directory
git add <gitlink>
git commit <gitlink> -m "Add submodule"
# me: "Oh crap! I did forget the configuration."
git submodule add --force <url> <gitlink>
# Git: "It already exists in the index, I am not going to produce the config for you."

The last step is changed with this patch, as
it will just work fine then.

Heiko Voigt

2016-10-11 16:38 UTC #5 lore raw

Re: [PATCH 1/2] submodule add: extend force flag to add existing repos

Hi,

Quoted 14 lines · Stefan Beller

Quoted 13 lines · Heiko Voigt
Quoted 11 lines · Junio C Hamano
Quoted 9 lines · Stefan Beller
Currently the force flag in git submodule add takes care of possibly ignored files or when a name collision occurs.
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was done (i.e. no .gitmodules file nor any entry in .git/config) and you want to generate these config entries. For this situation allow git submodule add to proceed if there is already a submodule at the given path in the index.

Quoted 3 lines

Quoted 1 line
Is it important that the submodule is in the index?
If it is not in the index, it already works.

Ah ok I was not aware of that, sorry.

Quoted 10 lines

Quoted 2 lines
How about worktree?
From the index entry alone we can not deduce the values anyway.
Right, but as of now this is the only show stopper, i.e.
* you have an existing repo? -> fine, it works with --force
* you even ignored that repo -> --force knows how to do it.
* you already have a gitlink -> Sorry, you're out of luck.
So that is why I stressed index in this commit message, as it is only about this case.

Forget what I wrote. As said above I was not aware that there is only an error when it is already in the index.

Quoted 17 lines

Quoted 1 line
[1] http://public-inbox.org/git/%[email protected]%3E/
Current situation:
Quoted 3 lines
clone the submodule into a directory
git submodule add --force
git commit everything
works fine, but:
Quoted 6 lines
clone the submodule into a directory
git add <gitlink>
git commit <gitlink> -m "Add submodule"
# me: "Oh crap! I did forget the configuration."
git submodule add --force <url> <gitlink>
# Git: "It already exists in the index, I am not going to produce the config for you."

Quoted 2 lines

The last step is changed with this patch, as
it will just work fine then.

Thanks.

Cheers Heiko

Stefan Beller

author

2016-10-06 19:37 UTC #6 lore raw

PATCH v4 2/2 push: change submodule default to check when submodules exist

In reply to Stefan Beller

When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake.

Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing.

However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .git/modules directory. That directory doesn't exist when no submodules are used and is only created and populated when submodules are cloned/added.

When the submodule directory doesn't exist, a user may have changed the gitlinks via plumbing commands. Currently the default is to not check. RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently, though it may change in the future. When no submodules exist such a check is pointless as it would fail anyway, so let's just turn it off.

[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/

Signed-off-by Stefan Beller <[email protected]>

---
 builtin/push.c                 | 15 ++++++++++++++-
 t/t5531-deep-submodule-push.sh |  6 +++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

builtin/push.c +14 -1

index 3bb9d6b..683f270 100644
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "refs.h"
+#include "dir.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"

@@ -22,6 +23,7 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
+static int has_submodules_configured;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static enum transport_family family;

@@ -31,6 +33,15 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static void preset_submodule_default(void)
+{
+	if (has_submodules_configured || file_exists(git_path("modules")) ||
+	    (!is_bare_repository() && file_exists(".gitmodules")))
+		recurse_submodules = RECURSE_SUBMODULES_CHECK;
+	else
+		recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;

@@ -495,7 +506,8 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		const char *value;
 		if (!git_config_get_value("push.recursesubmodules", &value))
 			recurse_submodules = parse_push_recurse_submodules_arg(k, value);
-	}
+	} else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+		has_submodules_configured = 1;
 
 	return git_default_config(k, v, NULL);
 }

@@ -552,6 +564,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+	preset_submodule_default();
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);

t/t5531-deep-submodule-push.sh +5 -1

index 198ce84..e690749 100755
@@ -65,7 +65,11 @@ test_expect_success 'push fails if submodule commit not on remote' '
 		git add gar/bage &&
 		git commit -m "Third commit for gar/bage" &&
 		# the push should fail with --recurse-submodules=check
-		# on the command line...
+		# on the command line. "check" is the default for repos in
+		# which submodules are detected by existence of config,
+		# .gitmodules file or an internal .git/modules/<submodule-repo>
+		git submodule add -f ../submodule.git gar/bage &&
+		test_must_fail git push ../pub.git master &&
 		test_must_fail git push --recurse-submodules=check ../pub.git master &&
 
 		# ...or if specified in the configuration..

Signature · 1 line

2.10.1.353.g1629400

Junio C Hamano

2016-10-06 20:26 UTC #7 lore raw

Re: [PATCHv4 2/2] push: change submodule default to check when submodules exist

Quoted 11 lines · Stefan Beller

When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake.
Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing.
However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .git/modules directory.

... and other things like .gitmodules, submodule stuff in .git/config, etc.?

builtin/push.c static int git_push_config(const char *k, const char *v, void *cb) · line 510

Quoted 2 lines

builtin/push.c +2 -0

+	} else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+		has_submodules_configured = 1;

An in-code comment to explain why ".url" is so special would be helpful. Documentation/config.txt says .path and .url are both initially populated by 'git submodule init', which might be outdated information that confuses readers of the above code.

Quoted 25 lines

builtin/push.c +1 -0

@@ -552,6 +564,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+	preset_submodule_default();
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);

t/t5531-deep-submodule-push.sh +5 -1

index 198ce84..e690749 100755
@@ -65,7 +65,11 @@ test_expect_success 'push fails if submodule commit not on remote' '
 		git add gar/bage &&
 		git commit -m "Third commit for gar/bage" &&
 		# the push should fail with --recurse-submodules=check
-		# on the command line...
+		# on the command line. "check" is the default for repos in
+		# which submodules are detected by existence of config,
+		# .gitmodules file or an internal .git/modules/<submodule-repo>
+		git submodule add -f ../submodule.git gar/bage &&
+		test_must_fail git push ../pub.git master &&
 		test_must_fail git push --recurse-submodules=check ../pub.git master &&
 
 		# ...or if specified in the configuration..

Stefan Beller

author

2016-10-06 21:30 UTC #8 lore raw

Re: [PATCHv4 2/2] push: change submodule default to check when submodules exist

Quoted 10 lines · Junio C Hamano

Quoted 8 lines · Stefan Beller
When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake.
Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing.

Quoted 6 lines

Quoted 3 lines
However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .git/modules directory.
... and other things like .gitmodules, submodule stuff in .git/config, etc.?

Oh right, I need to update this commit message to mention this, too

Quoted 6 lines

builtin/push.c static int git_push_config(const char *k, const char *v, void *cb) · line 510
Quoted 3 lines
builtin/push.c +2 -0
+     } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+             has_submodules_configured = 1;
An in-code comment to explain why ".url" is so special would be helpful. Documentation/config.txt says .path and .url are both

submodule.<name>.path, submodule.<name>.url
The path within this project and URL for a submodule. These variables are initially populated by git submodule init. See git- submodule(1) and gitmodules(5) for details.

The correct version is:

submodule.<name>.path::
This is a config variable only found in .gitmodules files that
indicate at which
path relative to the repository root the submodule is found. It is used to resolve the mapping (submodule name)<->path throughout all time, i.e. also later when a submodule is initialized and checked out, any subsequent operation that needs a submodule name/repsoitory uses it for lookup.

submodule.<name>.url::
This is a config variable found both in .gitmodules files as well
as .git/config.
In the .gitmodules files it serves as a hint where to obtain a
repository from
to populate the gitlinks within the repository.
On submodule-init the value will be copied over to the .git/config
file and there
it serves as a holder of the value only until the first
submodule-update is run,
as the initial submodule-update is using that url to clone the submodule. From then on it is only used as a boolean flag in the .git/config
to indicate
whether the submodule is considered interesting for further submodule commands. (See the similar competing thing with submodule.<name>.ignore)

Given that it is obvious to my current me, that .url is the only sensible thing as .path
is not found in the .git/config which we are searching in here. So for the purpose of this patch I'd just add a line like this to remind my future self:

    /* See if any submodule is considered interesting: */

I would like to avoid references to .path as that is not helping here. (Why does that
comment point to a variable name that is not supposed to exist in the file?) So I think we really need to think how to reduce confusion in the future by readers that
have more or less overview of the submodules concept.

A future version of the man page may read:

submodule.<name>.path::
<same as before>, we may want to consider if we copy it over to
.git/config, so the
repository may trash the .gitmodules file and the submodules keep working.

submodule.<name>.url::
indication in .gitmodules where the submodule is cloned from. It
will be copied over
to .git/config on submodule-init. The user may adapt the
configured urls there before
proceeding to submodule-update, which will delete the url setting
again, as the
cloned submodule repository will hold the authoritative setting
where the remote
of the submodule is.
So it only exists in .git/config for submodules that are
initialized but have never
been cloned. Even when the submodule is deinit'd and reinited we
don't even need
to copy the url, as we still have the old submodule repository in
.git/module/<name>

submodule.<name>.exists::
This setting is per working tree (unlike the previous 2) that
indicates if submodule
related commands pay attention to the given submodule. It is still
unclear how this
works together with the .ignore setting.

Note for this future to come true, we'd have to have only 2 big series One that Duy started to introduce per working tree configs, see

    https://public-inbox.org/git/[email protected]/

The other one is a series that hasn't seen the mailing list yet, that I started to
separate the 2 modes of the url both as a value holder (to point at a URL) as well as just a boolean flag for commands to acknowledge the existence of the submodule.

Quoted 2 lines

initially populated by 'git submodule init', which might be outdated information that confuses readers of the above code.

A lot of text but I guess we can use some ideas from here to update the .path and .url documentation. (The wording in this email is not of man page quality).

Thanks,
Stefan

Stefan Beller

author

2016-10-06 23:41 UTC #9 lore raw

PATCH v5 1/1 push: change submodule default to check when submodules exist

In reply to Junio C Hamano

Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing.

However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of
* any initialized submodules via checking submodule.<name>.url.
* the .git/modules directory. That directory doesn't exist when no submodules are used and is only created and populated when submodules are cloned/added.
* the existence of the .gitmodules file.

[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/

Signed-off-by Stefan Beller <[email protected]>

---

* Reworded the commit message
* added comment on why we only check submodule.<name>.url

The first patch of the series is still good, so please use
https://public-inbox.org/git/[email protected]/
first and then build this on top.

Thanks,
Stefan

 builtin/push.c                 | 16 +++++++++++++++-
 t/t5531-deep-submodule-push.sh |  6 +++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

builtin/push.c +15 -1

index 3bb9d6b..9e0b8db 100644
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "refs.h"
+#include "dir.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"

@@ -22,6 +23,7 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
+static int has_submodules_configured;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static enum transport_family family;

@@ -31,6 +33,15 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static void preset_submodule_default(void)
+{
+	if (has_submodules_configured || file_exists(git_path("modules")) ||
+	    (!is_bare_repository() && file_exists(".gitmodules")))
+		recurse_submodules = RECURSE_SUBMODULES_CHECK;
+	else
+		recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;

@@ -495,7 +506,9 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		const char *value;
 		if (!git_config_get_value("push.recursesubmodules", &value))
 			recurse_submodules = parse_push_recurse_submodules_arg(k, value);
-	}
+	} else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+		/* The submodule.<name>.url is used as a bit to indicate existence */
+		has_submodules_configured = 1;
 
 	return git_default_config(k, v, NULL);
 }

@@ -552,6 +565,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+	preset_submodule_default();
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);

t/t5531-deep-submodule-push.sh +5 -1

index 198ce84..e690749 100755
@@ -65,7 +65,11 @@ test_expect_success 'push fails if submodule commit not on remote' '
 		git add gar/bage &&
 		git commit -m "Third commit for gar/bage" &&
 		# the push should fail with --recurse-submodules=check
-		# on the command line...
+		# on the command line. "check" is the default for repos in
+		# which submodules are detected by existence of config,
+		# .gitmodules file or an internal .git/modules/<submodule-repo>
+		git submodule add -f ../submodule.git gar/bage &&
+		test_must_fail git push ../pub.git master &&
 		test_must_fail git push --recurse-submodules=check ../pub.git master &&
 
 		# ...or if specified in the configuration..

Signature · 1 line

2.10.1.353.g1629400