[PATCH] Revert "push: change submodule default to check when submodules exist"

[PATCH] Revert "push: change submodule default to check when submodules exist"

[Stefan Beller]

This reverts commit 04a1d8b1ae5eeecb90675cfaca2850bf26269485.

In the previous commit we set push.recurseSubmodules to "check"
by default. This check is done by walking all remote refs that are known.

For remotes we only store the refs/heads/* (and tags), which doesn't
include all commits. In e.g. Gerrit commits often end up at refs/changes/*
(that we do not store) when pushing to refs/for/master (which we also do
not store). So a workflow such as the following still fails:

    $ git -C <submodule> push origin HEAD:refs/for/master
    $ git push origin HEAD:refs/for/master
    The following submodule paths contain changes that can
    not be found on any remote:
      submodule

    Please try

        git push --recurse-submodules=on-demand

    or cd to the path and use

        git push

    to push them to a remote.

Trying to push with --recurse-submodules={on-demand,check}
would run into the same problem, yielding the same error message.

So changing the default to check for submodules is clearly not working
as intended for Gerrit users. We need another option that works for them.
For now just revert the change of the default.

A future patch to address this problem would be add another option that
treats the submodules differently:

  1) check if the submodule needs pushing (as currently done in "check")
  2) for those submodules that need pushing we run a command, e.g.
     git push with the refspec passed down exactly as is. This would
     work for the Gerrit case, as HEAD:refs/for/master is correct
     for both the superproject and the submodule.
  3) Unlike in "on-demand", we would not check again after performing
     step 2), as we would not find the newly pushed things at Gerrit.

Once we have such an option, we can default to "check" again, and the
error message would mention both on-demand as well as this new option.
I'd imagine "on-demand" is what works in branch driven code review
systems such as github; for Gerrit which is based on changes the option
outlined above would work.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

  After some thought, my opinion on
  sb/push-make-submodule-check-the-default
  change. We should not merge it down to master
  until we found a good solution.
  
  This applies on top of sb/push-make-submodule-check-the-default
  unbreaking existing Gerrit users, explaining why this was a bad idea.
  
  Feel free to either apply this patch or just eject said branch from
  next.
  
  Thanks,
  Stefan  

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

diff --git a/builtin/push.c b/builtin/push.c
index 9e0b8dba9a..3bb9d6b7e6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,7 +3,6 @@
  */
 #include "cache.h"
 #include "refs.h"
-#include "dir.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"
@@ -23,7 +22,6 @@ 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;
 
@@ -33,15 +31,6 @@ 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++;
@@ -506,9 +495,7 @@ 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);
 }
@@ -565,7 +552,6 @@ 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 e690749e8a..198ce84754 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -65,11 +65,7 @@ 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. "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 &&
+		# on the command line...
 		test_must_fail git push --recurse-submodules=check ../pub.git master &&
 
 		# ...or if specified in the configuration..
-- 
2.11.0.495.g04f60290a0.dirty