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

Stefan Beller [off-list ref] writes:

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

+	} 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.

On Thu, Oct 6, 2016 at 1:25 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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

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

quoted

+     } 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/20160720172419.25473-1-pclouds@gmail.com/
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.

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

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

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

 * 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/20161006193725.31553-2-sbeller@google.com/
 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(-)

diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..9e0b8db 100644
--- a/builtin/push.c
+++ b/builtin/push.c

@@ -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);

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 198ce84..e690749 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh

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

-- 
2.10.1.353.g1629400