Re: [PATCH 2/4] push: make upstream, simple work with pushdefault

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> rr/triangle (4d35924, 2013-04-07) introduced support for triangular
> workflows, but did not think through the effect of the new configuration
> variables in the upstream and simple modes.  When remote.pushdefault or
> branch.<name>.pushremote is set, and push.default is set to upstream or
> simple, this happens (master@{u} != origin):
>
>   $ 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.
>
> The configuration variables work as expected with push.default = current
> and matching, which makes the above unexpected and counter-intuitive.
> It happens because upstream and simple were designed with central
> workflows in mind.  Even when these configuration variables are not set,
>
>   $ git push origin
>   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.

I am not sure what you mean by artificial.

If you have push.default set upstream or simple, then a push run
while on the branch 'foo' will figure out what happens when you do a
fetch by looking at 'branch.foo.merge' to find the branch we are set
to integrate from 'branch.foo.remote' remote.  The simple further
says that branch name must be the same as 'foo'.

And that is what setup_push_UPSTREAM() is designed to do.  Rejecting
a call that breaks the precondition is perfectly the right thing to
do: if you set to push to "upstream" and if you are trying to push
to a different remote, for example.

The triangle topic changed the precondition without updating the
logic to check it, which was the bug, not the original check.

Also, it is still unclear to me why push.default = upstream in a
triangular setting should compute that the current branch 'foo' is
set to integrate with 'master' from the remote 'origin' and assume
that updating branch 'master' in a different repository, which may
or may not have anything to do with 'master' at 'origin', is a sane
thing to do.

We do not have the branch.$name.push yet and if we did, that would
be the value to be used.  So I agree that we need to default to
something, but I do not think this patch has justified that updating
'master' (i.e. branch.foo.merge) in a repository that is totally
unrelated to the branch branch.foo.merge's name was taken from is a
better default than updating 'foo' itself.  Both look like a random
choice at least to me.

I actually am OK with 'upstream' that rejects triangular, while
making 'simple' do something different [*1*].

"push.default = upstream" is to "push back to where I fetch and
integrate with", which by definition makes no sense in triangular
workflow.  But 'simple' will be the new default and it does not say
anything about "what I fetch and integrate with" at all, so we can
choose a sensible default when branch.$name.push does not say what
to update.


[Footnote]

*1* I offhand do not know what that "something else" is, but it
would probably be closer to 'current'.  In an extreme case, consider
that there may no branch.$name.remote or branch.$name.merge at all
(the user is the top-level integrator).

Removing the "remote name must match when using 'upstream'" check in
the setup_push_upstream() is not sufficient if we want to allow the
remote.pushdefault for such a user with push.default = simple mode,
because presense of branch.$name.remote and branch.$name.merge is
checked very early in the function, and lack of them will error out.