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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 6, 2016, 20:25 UTC
Message-ID: <xmqqoa2xi5rd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161006193725.31553-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 11 quoted lines

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

Show 25 quoted lines

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