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

Fredrik Gustafsson [off-list ref] 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.

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.

Am 28.07.2011 22:05, schrieb Junio C Hamano:

Fredrik Gustafsson [off-list ref] writes:

quoted

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?

Am 28.07.2011 22:05, schrieb Junio C Hamano:

Fredrik Gustafsson [off-list ref] writes:

quoted

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?