Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> When remote.pushdefault or branch.<name>.pushremote is set (a triangular
> workflow feature), master@{u} != origin, and push.default is set to
> `upstream` or `simple`:
>
>   $ git push
>   fatal: You are pushing to remote 'origin', which is not the upstream of
>   your current branch 'master', without telling me what to push
>   to update which remote branch.
>
> Unfortunately, in the case of `upstream`, the very name indicates that
> it is only suitable for use in central workflows; let us not even
> attempt to give it a new meaning in triangular workflows, and error out
> as usual.

Sensible.

> However, the `simple` does not have this problem: it is poised to
> be the default for Git 2.0, and we would definitely like it to do
> something sensible in triangular workflows.
>
> Decouple `simple` from `upstream` completely, and change it to mean
> `current` with a safety feature: a `push` and `pull` should not be
> asymmetrical in the special case of central workflows.

Double negation confused my parser.  'push' and 'pull' should be
kept symmetrical in central workflows?

> +* `simple` - a safer version of `current`; push the current branch to
> +  update a branch with the same name on the receiving end, with a
> +  safety feature: in central workflows, error out if
> +  branch.$branch.merge is set and not equal to $branch,

If branch.$branch.merge is _not_ set, what happens in the current
code, and what should happen?

> + to make sure
> +  that a `push` and `push` are never asymmetrical.  It will become the
> +  default in Git 2.0.

Ditto.

>  * `matching` - push all branches having the same name on both ends
>    (essentially ignoring all newly created local branches).
> diff --git a/builtin/push.c b/builtin/push.c
> index 2d84d10..d8d27d9 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -120,6 +120,25 @@ static const char message_detached_head_die[] =
>  	   "\n"
>  	   "    git push %s HEAD:<name-of-remote-branch>\n");
>  
> +static void setup_push_simple(struct remote *remote)
> +{
> +	struct branch *branch = branch_get(NULL);
> +	if (!branch)
> +		die(_(message_detached_head_die), remote->name);

OK.

> +	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
> +		/* No upstream configured */
> +		goto end;

Without any configuration the current branch is pushed out, which
loosens the safety we implemented in the current 'safer upstream'.

I am not convinced this is a good change.  I am not convinced this is
a bad change, either, yet, but this loosening smells bad.

> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index 69ce6bf..e54dd02 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -85,7 +85,7 @@ test_expect_success 'push from/to new branch with current creates remote branch'
>  test_expect_success 'push to existing branch, with no upstream configured' '
>  	test_config branch.master.remote repo1 &&
>  	git checkout master &&
> -	test_push_failure simple &&
> +	test_push_success simple master &&
>  	test_push_failure upstream
>  '

Likewise.