Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Semyon Perepelitsa]

I noticed the option in the man-page but there is still no configuration option available. Did you forget to add it after all? Right now --recurse-submodules has little use by itself as the problem it solves is forgetting to push a submodule which is no different from forgetting to specify the option.

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Jens Lehmann]

Hi Semyon,

Am 18.02.2014 18:49, schrieb Semyon Perepelitsa:
> I noticed the option in the man-page but there is still no configuration option available. Did you forget to add it after all?

Nope, we just didn't implement it yet. ;-)

It's one of the topics on my submodule ToDo list:

  https://github.com/jlehmann/git-submod-enhancements/wiki

> Right now --recurse-submodules has little use by itself as the problem it solves is forgetting to push a submodule which is no different from forgetting to specify the option.

I'm currently busy working on another submodule related topic. But
if you are interested, adding the "fetch.recurseSubmodules" config
option in be254a0e and the "submodule.<name>.fetchRecurseSubmodules"
option in c1a3c364 should be good examples of how to add the
"push.recurseSubmodules" and "submodule.<name>.pushRecurseSubmodules"
config options.

I'll be glad to help in bringing this topic forward by discussing
ideas and reviewing patches. And I believe we should change the
default behavior of push (which is currently quiet about unpushed
submodule commits as you noticed) to either "check" or "on-demand"
(with my preference currently being slightly on the former, but I'm
happy to be convinced otherwise ;-).


Thanks
Jens

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> We will skip the check for submodules without remotes, does that lessen
> your annoyance? We still think it is a good idea to have that test enabled
> by default, but it might be a good idea to wait with that until we provide
> a central config option to enable users to turn that off with a simple
> "git config push.recurseSubmodules off". What do you think?

I would rather see a questionable (in the sense that it can trigger false
positive and annoy users of workflows you didn't anticipate) new feature
turned off by default, so that interested people who _do_ want to can use
it knowing what they are getting into, to find out as much unexpected
fallout as possible. So if we want to give this topic early graduation, I
would suggest making it a command line option that defaults to off first,
then in a later commit add a configuration option to enable people to turn
it on (and allow those who do not see the point of the new feature to keep
it turned off permanently). Enabling the feature by default can be done
in a later version after the feature proves to be truly useful for people
who opted into the experiment.

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Jens Lehmann]

Am 28.07.2011 22:05, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
>> diff --git a/transport.h b/transport.h
>> index 161d724..c6ccf8c 100644
>> --- a/transport.h
>> +++ b/transport.h
>> @@ -101,6 +101,7 @@ struct transport {
>>  #define TRANSPORT_PUSH_MIRROR 8
>>  #define TRANSPORT_PUSH_PORCELAIN 16
>>  #define TRANSPORT_PUSH_SET_UPSTREAM 32
>> +#define TRANSPORT_PUSH_NO_RECURSE_SUBMODULES 64
> 
> Also naming the constant with NO will invite an unnecessary double
> negation, like this:
> 
> 	if (!(flags & FROTZ_NO_NITFOL))
>         	do_nitfol_to_frotz();

Right, we'll drop the "NO_" from that constant.

> Besides, I would be moderately annoyed if this check were the default.

We will skip the check for submodules without remotes, does that lessen
your annoyance? We still think it is a good idea to have that test enabled
by default, but it might be a good idea to wait with that until we provide
a central config option to enable users to turn that off with a simple
"git config push.recurseSubmodules off". What do you think?

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Jens Lehmann]

Am 28.07.2011 22:05, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
>> This adds the option --no-recurse-submodules to push. That is, git
> 
> I think this needs to be renamed at least for two reasons.
> 
> The name makes it sound as if "push --recurse-submodules" would
> recursively visit the submodules and runs "push" there, but I do not think
> that is what this flag does.

That is because the patch that does this is still in the making ;-)

The cover letter should have mentioned it, but we talked about making push
pretty symmetric to fetch:

- Use "--no-recurse-submodules" if you don't want submodules to be pushed
  (we added that right now so users can disable the behavior the second
  commit introduces)
- Use "--recurse-submodules=on-demand" to push only those submodules where
  new commits have been recorded in the superproject's refs to be pushed
- Use "--recurse-submodules" to unconditionally push everything in the
  submodules too
- Make the default configurable by a "push.recurseSubmodules" option

We'll need another round to discuss how to handle private submodules which
were never intended to be pushed, but I think the general idea of having
fetch and push use similar options makes sense, no?

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> This adds the option --no-recurse-submodules to push. That is, git

I think this needs to be renamed at least for two reasons.

The name makes it sound as if "push --recurse-submodules" would
recursively visit the submodules and runs "push" there, but I do not think
that is what this flag does.

> diff --git a/transport.h b/transport.h
> index 161d724..c6ccf8c 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -101,6 +101,7 @@ struct transport {
>  #define TRANSPORT_PUSH_MIRROR 8
>  #define TRANSPORT_PUSH_PORCELAIN 16
>  #define TRANSPORT_PUSH_SET_UPSTREAM 32
> +#define TRANSPORT_PUSH_NO_RECURSE_SUBMODULES 64

Also naming the constant with NO will invite an unnecessary double
negation, like this:

	if (!(flags & FROTZ_NO_NITFOL))
        	do_nitfol_to_frotz();

Besides, I would be moderately annoyed if this check were the default.

[PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Fredrik Gustafsson]

This adds the option --no-recurse-submodules to push. That is, git
will not check if the submodules are pushed. -f or --force still also
disables this check. Documentation is also updated.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Documentation/git-push.txt     |    6 ++++++
 builtin/push.c                 |    1 +
 t/t5531-deep-submodule-push.sh |    7 +++++++
 transport.c                    |    4 +++-
 transport.h                    |    1 +
 5 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 88acfcd..d63a57c 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -113,6 +113,8 @@ nor in any Push line of the corresponding remotes file---see below).
 	not an ancestor of the local ref used to overwrite it.
 	This flag disables the check.  This can cause the
 	remote repository to lose commits; use it with care.
++
+	This also enforces --no-recurse-submodules
 
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
@@ -162,6 +164,10 @@ useful if you write an alias or script around 'git push'.
 	is specified. This flag forces progress status even if the
 	standard error stream is not directed to a terminal.
 
+--no-recurse-submodules::
+	Don't check if all submodule commits this repository refers to are
+	pushed to their remotes.
+
 include::urls-remotes.txt[]
 
 OUTPUT
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..07a8b11 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -236,6 +236,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
+		OPT_BIT(0, "no-recurse-submodules", &flags, "do not recurse submodules", TRANSPORT_PUSH_NO_RECURSE_SUBMODULES),
 		OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
 		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 15474c1..74c615c 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -124,4 +124,11 @@ test_expect_success 'push fails if submodule has no remote and is on the first s
 	)
 '
 
+test_expect_success 'push succeeds when --no-recurse-submodules is used' '
+	(
+		cd work &&
+		git push ../pub.git --no-recurse-submodules
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index e0fd435..9681560 100644
--- a/transport.c
+++ b/transport.c
@@ -1042,7 +1042,9 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
 
-		if(!(flags & TRANSPORT_PUSH_FORCE) && !is_bare_repository()) {
+		if(!(flags & TRANSPORT_PUSH_NO_RECURSE_SUBMODULES) &&
+		   !(flags & TRANSPORT_PUSH_FORCE) &&
+		   !is_bare_repository()) {
 			struct ref *ref = remote_refs;
 			for (; ref; ref = ref->next)
 				if(!is_null_sha1(ref->new_sha1) && check_for_unpushed_submodule_commits(ref->new_sha1))
diff --git a/transport.h b/transport.h
index 161d724..c6ccf8c 100644
--- a/transport.h
+++ b/transport.h
@@ -101,6 +101,7 @@ struct transport {
 #define TRANSPORT_PUSH_MIRROR 8
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_PUSH_NO_RECURSE_SUBMODULES 64
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.6.236.g7ad21