Re: [PATCH v2] New config push.default to decide default behavior for push

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2009, 00:48 UTC
Message-ID: <7vwsavlg6q.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090311220144.GA6782@pvv.org>

Finn Arne Gangstad <finnag@pvv.org> writes:

Show 27 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f5152c5..6fdf829 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1160,6 +1160,25 @@ pull.octopus::
>  pull.twohead::
>  	The default merge strategy to use when pulling a single branch.
>  
> +push.default::
> +	Defines the action git push should take if no refspec is given
> +	on the command line, no refspec is configured in the remote, and
> +	no refspec is implied by any of the options given on the command
> +	line.
> +
> +	The term `current remote` means the remote configured for the current
> +	branch, or `origin` if no remote is configured. `origin` is also used
> +	if you are not on any branch.
> +
> +	Possible values are:
> ++
> +* `nothing` do not push anything.
> +* `matching` push all matching branches to the current remote.
> +  All branches having the same name in both ends are considered to be
> +  matching. This is the current default value.
> +* `tracking` push the current branch to whatever it is tracking.
> +* `current` push the current branch to a branch of the same name.
> +

I thought I fixed asciidoc formatting around this part in the version I queued in 'pu'; in any case, the second and subsequent paragraphs need dedenting.

> +static void warn_unconfigured_push()
> +{

I think I also fixed this "old style declaration".

Show 26 quoted lines

> +static void do_default_push(struct remote *remote)
> +{
> +	git_config(git_default_config, NULL);
> +	switch (push_default) {
> +	case PUSH_DEFAULT_UNSPECIFIED:
> +		warn_unconfigured_push();
> +		/* fallthrough */
> +
> +	case PUSH_DEFAULT_MATCHING:
> +		add_refspec(":");
> +		break;
> +
> +	case PUSH_DEFAULT_TRACKING:
> +		setup_push_tracking(remote);
> +		break;
> +
> +	case PUSH_DEFAULT_CURRENT:
> +		add_refspec("HEAD");
> +		break;
> +
> +	case PUSH_DEFAULT_NOTHING:
> +		die("Nothing to push, and push is configured to push nothing "
> +		    "by default.");
> +		break;
> +	}
> +}

This part looks good, provided if we were to go ahead and plan to change the default in the future.

Show 18 quoted lines

> +static void setup_push_tracking(struct remote *remote)
> +{
> +	int n;
> +	struct branch *branch = branch_get(NULL);
> +	if (!branch)
> +		die("You are not currently on a branch.");
> +	if (!branch->merge_nr)
> +		die("The current branch %s is not tracking anything.",
> +		    branch->name);
> +	if (branch->remote != remote)
> +		die("The current branch is tracking \"%s\", not \"%s\"!",
> +		    branch->remote->name, remote->name);
> +	for (n = 0; n < branch->merge_nr; n++) {
> +		struct strbuf rs = STRBUF_INIT;
> +		strbuf_addf(&rs, "%s:%s", branch->name, branch->merge[n]->src);
> +		add_refspec(rs.buf);
> +	}
> +}

If a branch is configured to merge from multiple places (e.g. testing branch similar to the linux-next tree to integrate from multiple staging trees), a sane default would be not to push it out to any of the branches it pulls from---it is a consumer to the other branches, and it is meant to be sent to somewhere else, not back to any of the originators. Instead, this code will push to all of them, which I would not see any sane use case for. It might make a bit sense if you refused unless merge_nr is exactly one.

Also I am not sure if the check between the name of the remote makes much practical sense. Many people use two remotes to name the same one for pushing over ssh and fetching over git. Which name comes in which? I think with this logic you are trying to catch a mistake like:

    $ git push --tracking $there

when the current branch tracks a remote branch that did not come from the remote repository $there but somewhere else, but if that is the motivation behind it, does it help to forbid "push --tracking" to take any destination repository to reduce such a confusion?