[PATCH 0/4] Fix triangular workflows for upstream, simple

[PATCH 0/4] Fix triangular workflows for upstream, simple

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> It might be a beginning of a change in the right direction (I didn't
> check the codeflow), but given that the test that comes after the
> one you removed is looking at branch->merge[0] and deciding what to
> do, and branch.$name.merge should *never* affect anything when
> remote.pushdefault or branch.$name.pushremote is in use while
> deciding what is pushed out or if the push is allowed, I do not
> think the removal of these five lines alone can possibly "work".

I just spent hours breaking my head thinking about why you said my
patch cannot possibly "work".  I give up.

The artificial limitation was introduced by you in 135dade, and here's
the relevant part of the commit message:

  - The current branch does have its remote and upstream branch configured,
    but the user said "git push $there", where $there is not the remote
    named by "branch.$name.remote".  By definition, no branch at repository
    $there is set to integrate with the current branch in this case, and
    this push is not meant to update any branch at the remote repository
    $there.

What definition is this, and why does it only apply to
upstream/simple?  Did you implicitly assume that upstream/simple are
only meant to be used in centralized workflows?

Thanks.

Ramkumar Ramachandra (4):
  t/push-default: remove redundant test_config lines
  push: make upstream, simple work with pushdefault
  t/push-default: generalize test_push_{success, commit}
  t/push-default: test pushdefault with all modes

 builtin/push.c          |  5 -----
 t/t5528-push-default.sh | 30 ++++++++++++++++++++++++------
 2 files changed, 24 insertions(+), 11 deletions(-)

-- 
1.8.3.247.g485169c

[PATCH 1/4] t/push-default: remove redundant test_config lines

[Ramkumar Ramachandra]

The line

  test_config push.default upstream

appears unnecessarily in two tests, as the final test_push_failure sets
push.default before pushing anyway.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5528-push-default.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 4736da8..69ce6bf 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -48,7 +48,6 @@ test_expect_success '"upstream" pushes to configured upstream' '
 test_expect_success '"upstream" does not push on unconfigured remote' '
 	git checkout master &&
 	test_unconfig branch.master.remote &&
-	test_config push.default upstream &&
 	test_commit three &&
 	test_push_failure upstream
 '
@@ -57,7 +56,6 @@ test_expect_success '"upstream" does not push on unconfigured branch' '
 	git checkout master &&
 	test_config branch.master.remote parent1 &&
 	test_unconfig branch.master.merge &&
-	test_config push.default upstream
 	test_commit four &&
 	test_push_failure upstream
 '
-- 
1.8.3.247.g485169c

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

[Ramkumar Ramachandra]

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.

This artificial limitation imposed on setup_push_upstream() was
introduced by 135dad (push: error out when the "upstream" semantics does
not make sense, 2012-03-30), but has no basis; a push.default value
should only dictate a refspec, and the push destination is orthogonal to
this.  Remove this artificial limitation, fixing the bug.  Only one test
needs to be changed subtly.

Reported-by: Leandro Lucarella <leandro.lucarella@sociomantic.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c          | 5 -----
 t/t5528-push-default.sh | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..b253a64 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -137,11 +137,6 @@ static void setup_push_upstream(struct remote *remote, int simple)
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
-	if (strcmp(branch->remote_name, remote->name))
-		die(_("You are pushing to remote '%s', which is not the upstream of\n"
-		      "your current branch '%s', without telling me what to push\n"
-		      "to update which remote branch."),
-		    remote->name, branch->name);
 	if (simple && strcmp(branch->refname, branch->merge[0]->src))
 		die_push_simple(branch, remote);
 
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 69ce6bf..4e4824e 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -60,13 +60,13 @@ test_expect_success '"upstream" does not push on unconfigured branch' '
 	test_push_failure upstream
 '
 
-test_expect_success '"upstream" does not push when remotes do not match' '
+test_expect_success '"upstream" pushes when remotes do not match' '
 	git checkout master &&
 	test_config branch.master.remote parent1 &&
 	test_config branch.master.merge refs/heads/foo &&
 	test_config push.default upstream &&
 	test_commit five &&
-	test_must_fail git push parent2
+	git push parent2
 '
 
 test_expect_success 'push from/to new branch with upstream, matching and simple' '
-- 
1.8.3.247.g485169c

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

[Junio C Hamano]

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.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I am not sure what you mean by artificial.

By artificial, I mean that the precondition is absolutely unnecessary
for the code following it to work.  The precondition was introduced in
a separate commit, specifically denying one usecase because the author
(you) thought that made sense.

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

I understand this perfectly well, as evidenced by the tests I've
written out in 4/4.

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

Did I claim otherwise?  :)
The topic is about fixing a bug introduced by rr/triangle.

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

Okay, so you haven't outlined a solution either.  Like I said in the
cover-letter, I've spent hours breaking my head and can't figure out a
better solution.  The bigger problem is that upstream/ simple were
designed with only central workflows in mind.  How do we deal with it
now?

Yes, upstream/ simple work only when @{u} resolves, and I haven't
changed this (because we don't have a well-defined meaning for what
branch.<name>.merge without a branch.<name>.remote means).  My
argument is very simple: no push.default mode should not dictate the
push destination; only the push refspec.  What makes sense to the user
is an upstream/ simple that works as expected with pushdefault: do the
tests I've outlined in 4/4 not make sense?  *scratches head*

I don't understand why upstream/ simple should _not_ push to a
different destination from the one being merged from.  I'll repeat:
push source/ destination is orthogonal to push refspec, and
push.default modes dictate the refspec.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I don't understand why upstream/ simple should _not_ push to a
> different destination from the one being merged from.  I'll repeat:
> push source/ destination is orthogonal to push refspec, and
> push.default modes dictate the refspec.

That is where we differ.  You keep repeating that without justifying
why reusing the branch upstream would have chosen in a non-triangular
workflow is a good idea in the first place.

Let's step back a bit and think.  In a workflow where we both agree
that "upstream" mode makes sense, i.e. central repository workflow,
we may have a "triangle" topic in our shared repository, and I may
have my own topic forked from it (i.e. set to fetch that "triangle"
and integrate my work with it).  The topic is to work on one aspect
of triangular workflow, e.g. "pushbranch" (i.e. I am not worried
about how we choose which remote to push to, but interested in what
branch at that remote is updated).

It makes sense for me to push the result back to the upstream,
i.e. "triangle" branch at the shared remote where I forked from, to
collaborate with other people (e.g. you) whose shared purpose is to
improve the triangular workflow.  You may be doing the same, and
your local branch may be called "triangle" (bag of various
triangular workflow improvements), or "pushremote" (a specific
subtopic).  In either case, you push to "upstream" to update
"triangle".

What should happen in a triangular workflow, where I may be fetching
and integrating from your repository to work on triangular workflow
in general (let's say that you are the owner of that topic) to
advance the support for that workflow.  The topic in your repository
may be called "triangle" to improve triangular workflow in general,
and again my branch would be "pushbranch" to improve one specific
aspect of it.

As we are using triangular workflow, the place *I* push to is under
my control, not the shared maintainer one or your repository, and
I'll be requesting you to pull that topic from me.

Now think _why_ I renamed the branch on my end, i.e. not calling
that branch in question "triangle" that is the blanket name for the
collective effort but calling it with a more specific name
"pushbranch", in the first place.

It is to clarify that my part of the effort is that particular
subarea of the topic.  Wouldn't it be a lot more sensible to use
that more specific name when publishing, rather than "triangle"
name, and ask you to pull that branch that describes what I
concentrated on?

Push destination and fetch source, when they are different, refer to
different repositories by definition, and they are in different
administration domains.  Why should the name used to publish my work
which is a specialization of what I built on (i.e. your "triangle")
lose the specialization from its name when I push it out?  We can
afford not to lose information in the branch name.

The difference between the shared repository case and triangular
workflow primarily comes from the fact that the final integration
happens at a different place.  In the shared repository case, the
integration happens in each participant's local repository and push
updates the shared history at the known name.  If my "pushbranch"
specialization diverted from the overall "triangle" effort, I would
first integrate others work and then update the remote, and at that
point, what I am pushing is no longer an isolated specialized effort,
but the result of its integration into a larger topic.

And once the more specialized topic is integrated into a more
generic branch it was forked from, it makes sense to update that
more generic branch at the shared repository.

In the triangular case, however, you as the "triangle" topic owner
has the choice to fetch and integrate from me or not to do so.  And
the integration happens when you decide to fetch and integrate.
There is no reason whatsoever to lose the more specific name until
that integration happens into your (meaning: the owner of the more
general "triangle" topic) "bag of various triangular improvements"
topic.

Imagine the case I forked two branches from your "triangle" topic
and pushing them to my publishing repository using the triangular
workflow.  Why should I attempt to update a single "triangle" branch
with both of these topics?

The name under which the local branch is published needs a sensible
default (when branch.$name.push is not specified), and I agree that
you would get the name of the branch my work was forked from if you
reuse the "upstream" code.  I am saying that it does not necessarily
give us a good default.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The name under which the local branch is published needs a sensible
> default (when branch.$name.push is not specified), and I agree that
> you would get the name of the branch my work was forked from if you
> reuse the "upstream" code.  I am saying that it does not necessarily
> give us a good default.

See, that's the problem: push.default is simply a default refspec to
push to for all branches, that can be _overridden_ by
branch.<name>.push.  Getting an override is not going to solve the
problem we are facing: what to do when branch.<name>.push is
unspecified?  Fall back to push.default, right?

> Now think _why_ I renamed the branch on my end, i.e. not calling
> that branch in question "triangle" that is the blanket name for the
> collective effort but calling it with a more specific name
> "pushbranch", in the first place.

Look, it's very simple.  upstream was built to support the case when
the local branch name is different from the remote branch name, but it
was too specialized for central workflows.  How do we extend it for
triangular workflows?

Just like we introduced branch.<name>.pushremote to override
branch.<name>.remote, we get branch.<name>.push to override
branch.<name>.merge.  If branch.<name>.pushremote is unset, where do
the pushes go?  branch.<name>.remote.  If branch.<name>.push is
unspecified, what is the refspec to be pushed?  branch.<name>.merge
(when push.default = upstream) [*1*].

What does this mean?  I publish the branch "triangle" on ram (what my
local branch is called or what push.default I use is irrelevant).  You
have a branch called pushremote with branch.pushremote.remote set to
ram, remote.pushdefault set to junio, branch.pushremote.merge set to
refs/heads/triangle, and push.default set to upstream.

  # on jc's machine; on branch pushremote
  $ git pull
  # integrates changes from ram's triangle just fine
  $ git push
  # publishes the branch as triangle on remote junio

I rewrite my branch, and you have 4 commits based on my branch:

  $ git rebase --onto @{u} @~4

Perfect.

The only limitation is that you don't choose the branch name on junio.
 When we get branch.pushremote.push, you'll be able to set it to
refs/heads/pushremote, making push.default inconsequential.  Done.

[Footnotes]

*1* remote.pushdefault overrides branch.<name>.remote, while
push.default will be overridden by a future branch.<name>.push.  Not
exactly elegant, is it?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> The name under which the local branch is published needs a sensible
>> default (when branch.$name.push is not specified), and I agree that
>> you would get the name of the branch my work was forked from if you
>> reuse the "upstream" code.  I am saying that it does not necessarily
>> give us a good default.
>
> See, that's the problem: push.default is simply a default refspec to
> push to for all branches, that can be _overridden_ by
> branch.<name>.push.

Think again and remember that Git is distributed.

As you say, the push.default configuration specifies the mode that
computes what remote branches are updated.  Where we differ is that
I consider "triangle" branch at your repository I fetch from and
"triangle" branch at the repository I push to (in the triangular
workflow) different entities, just like "triangle" branch in my
local repository is different from both.  branch.triangle.merge
configuration is used to link the first and the third, and the first
is called "upstream" of the third for this reason.  It does not have
anything to do with the second.  You seem to think branch.*.merge is
used to link the second with the first, and I do not think it makes
any sense.

> Getting an override is not going to solve the
> problem we are facing: what to do when branch.<name>.push is
> unspecified?  Fall back to push.default, right?

That is not the issue.  We both are solving it by falling back to
push.default, but the problem is that the mental picture you have
for push.default has not been updated to support the triangular
world better.  In the centralized world, the first and the second
(in the above description) remote branches, both of which are named
"triangle", are the same, because they are in the same remote
repository.  Trying to reuse that in the triangular world is *not*
automatically being orthogonal and better; it may be simply keeping
things inconvenient by choosing a wrong definition, which is what I
am trying to avoid by thinking things through.

>> Now think _why_ I renamed the branch on my end, i.e. not calling
>> that branch in question "triangle" that is the blanket name for the
>> collective effort but calling it with a more specific name
>> "pushbranch", in the first place.
>
> Look, it's very simple.  upstream was built to support the case when
> the local branch name is different from the remote branch name, but it
> was too specialized for central workflows.  How do we extend it for
> triangular workflows?

"upstream" is for the case where you push back to your "upstream",
that which you fetch from and integrate with.  It is perfectly fine
to extend it for triangular workflow by erroring it out when it is
used, as you do *not* want to update "upstream" in that workflow.
You are sending your work to your publishing repository.

> Just like we introduced branch.<name>.pushremote to override
> branch.<name>.remote, we get branch.<name>.push to override
> branch.<name>.merge.  If branch.<name>.pushremote is unset, where do
> the pushes go?  branch.<name>.remote.  If branch.<name>.push is
> unspecified, what is the refspec to be pushed?  branch.<name>.merge
> (when push.default = upstream) [*1*].
>
> What does this mean?

Doesn't it just mean that you do not understand "distributed"?  

What makes you think that the repository you fetch from and the
repository you push to, which are by definition different
repositories, have to share the same namespace?

>  I publish the branch "triangle" on ram (what my
> local branch is called or what push.default I use is irrelevant).  You
> have a branch called pushremote with branch.pushremote.remote set to
> ram, remote.pushdefault set to junio, branch.pushremote.merge set to
> refs/heads/triangle, and push.default set to upstream.
>
>   # on jc's machine; on branch pushremote
>   $ git pull
>   # integrates changes from ram's triangle just fine
>   $ git push
>   # publishes the branch as triangle on remote junio

Only if I want to publish the result of the work forked from your
"triangle" as my "triangle", but that is not the case.  A fork to be
integrated by other is by definition more specialized than the
original, and I would publish my "pushbranch" subtopic as such, not
as "triangle".

It appears that the message you are responding to failed to convey
the primary point, so it may not be very useful for me to repeat it
here, but anyway.

> [Footnotes]
>
> *1* remote.pushdefault overrides branch.<name>.remote, while
> push.default will be overridden by a future branch.<name>.push.  Not
> exactly elegant, is it?

But that is not what is happening, unless you keep thinking
"push.default decides the name of the branch regardless of what
repository it lives in", which is where our difference lies, I
think.

Imagine the case where I forked two branches from your "triangle"
topic and pushing them to my repository using the triangular
workflow.  By your definition, they will both try to push to
"triangle", which means you, the "triangle" topic maintainer, cannot
receive two independent pull requests from me.  You can only get a
single branch that pre-merges both, and if you want to get one but
not the other, you have to do the untangling of these two topics.

The solution to the above becomes clear and simple, once you stop
thinking that the branch namespace you have has to be the same as
the branch namespaces other people have (which was limitation
imposed by central workflow, to which triangular has no reason to be
bound to).  Pushing to "upstream" by tying the name I fetch from and
the name I publish the result as is not appropriate in triangular
workflow, so just fail it.

I am not saying that you have to pick one to use for push.default
among the remaining ones (i.e. matching, current, what else?).  It
is very plausible that the triangular workflow wants a different
logic to pick what branches are to be updated and how.  Perhaps we
would want something that is capable of mapping your local branch
name to a branch name suitable in your publishing repository, and I
am not opposed to have such a mode.

But I think it _is_ a short-sighted mistake to consider "upstream"
the appropriate choice, if the reason to think so is only because
that is the only one that does any form of mapping in the current
system that was specifically designed to support workflow that
pushes back to where you fetched from to integrate with
(e.g. centralized).  We need to first ask if its mapping (i.e. what
the branch at "upstream" repository you merge into your local branch
in question is named) is the appropriate mapping to be used also for
pushing.  And I asked that question in the message you are
responding to (and my answer is it is not suitable).

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Only if I want to publish the result of the work forked from your
> "triangle" as my "triangle", but that is not the case.  A fork to be
> integrated by other is by definition more specialized than the
> original, and I would publish my "pushbranch" subtopic as such, not
> as "triangle".

Okay, so it should _never_ push out as "triangle", because it is an
insane default.

> But that is not what is happening, unless you keep thinking
> "push.default decides the name of the branch regardless of what
> repository it lives in", which is where our difference lies, I
> think.

I assumed that we want all push.default modes to do something
"sensible" in both central and triangular workflows.  push.default
dictating a push destination would break triangular workflows.  It's
very clear to me.

> Imagine the case where I forked two branches from your "triangle"
> topic and pushing them to my repository using the triangular
> workflow.  By your definition, they will both try to push to
> "triangle", which means you, the "triangle" topic maintainer, cannot
> receive two independent pull requests from me.  You can only get a
> single branch that pre-merges both, and if you want to get one but
> not the other, you have to do the untangling of these two topics.

Okay, you've made your point.  Due to namespace conflict, the solution
I proposed is not at all a "sane" default.  It's just confusing and
Bad (TM).

> I am not saying that you have to pick one to use for push.default
> among the remaining ones (i.e. matching, current, what else?).  It
> is very plausible that the triangular workflow wants a different
> logic to pick what branches are to be updated and how.  Perhaps we
> would want something that is capable of mapping your local branch
> name to a branch name suitable in your publishing repository, and I
> am not opposed to have such a mode.

Okay, we'll have to do some sort of split and mark push.default =
upstream/ simple suitable-only-for-centralized-workflows, or something
to that effect (deprecation?) :|

I'll try to think of something.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> I am not saying that you have to pick one to use for push.default
>> among the remaining ones (i.e. matching, current, what else?).  It
>> is very plausible that the triangular workflow wants a different
>> logic to pick what branches are to be updated and how.  Perhaps we
>> would want something that is capable of mapping your local branch
>> name to a branch name suitable in your publishing repository, and I
>> am not opposed to have such a mode.
>
> Okay, we'll have to do some sort of split and mark push.default =
> upstream/ simple suitable-only-for-centralized-workflows, or something
> to that effect (deprecation?) :|

Among the current ones, I think "upstream" is the only one that has
the "branch I fetch and integrate with is the one I want to update
with my result" connotation.  The "current" and "matching" modes
determine what gets pushed solely between the local repository you
are pushing from and the remote repository you are pushing to,
without getting "what do I fetch and integrate with" in the
equation.

As an extension to "upstream", the current implementation of
"simple" of course has the same issue, but because the name "simple"
does not inherently have such "branch I fetch and integrate with is
the one I want to update with my result" connotation, we can clean
up its semantics to match the new reality after triangular workflow.

If you recall the earlier discussion on "@{publish} which is
different from @{upstream}", one idea to allow mapping on the push
end was to introduce "push.default = single" that would act as
"upstream" when in "branch I fetch and integrate with is the same
branch at the same repository the one I want to update with my
result" workflow, and in a triangular workflow maps the branch being
pushed using remote.$name.push refspecs (if exists).

I think extending it further to act as 'current' if no push refspecs
are set for the remote you push to in a triangular workflow might be
a way to go if we want the mapping flexibility without having to set
branch.$name.push to each and every branch.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If you recall the earlier discussion on "@{publish} which is
> different from @{upstream}", one idea to allow mapping on the push
> end was to introduce "push.default = single" that would act as
> "upstream" when in "branch I fetch and integrate with is the same
> branch at the same repository the one I want to update with my
> result" workflow, and in a triangular workflow maps the branch being
> pushed using remote.$name.push refspecs (if exists).

I'm still resisting this idea, because I don't like these special-case
push.default modes.  If possible, I want to avoid introducing another
one to the existing mess.

> [...]

Okay, so what you're saying makes sense.  I'm cooking the following idea:

- current: push "$(HEAD)".  No restrictions on destination.  The most
generic, sensible, and extensible one, in my opinion.

- matching: push ":" to the destination specified by the current
branch. [since I cannot know what I'm pushing in advance, I think this
is generally ugly]

- upstream: In the special case when fetch source is equal to push
destination, push "$(HEAD):$(branch.$(HEAD).merge)".  Otherwise,
fallback to current.  Useful in central workflows.

- simple: [still haven't thought about what to do with this; I'm
generally not in favor of artificially crippling functionality by
erroring out]

Just like upstream respects branch.<name>.merge, current respects
branch.<name>.push, making branch-level ref mapping in triangular
workflows possible.  Finally, remote.<name>.push is entirely
orthogonal to all this, and is respected no matter what.

Am I making any sense?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>
>> [...]
>
> Okay, so what you're saying makes sense.

That is not a very useful style of quoting; what did you just agree to?

I think we should step back and clarify the "to which repository the
push goes" and "what branch(es) in that chosen repository is
updated".  The former is determined by your original "triangular"
topic in the recent world order.

The push.default specifies the logic/algorithm to decide the latter,
when there is no stronger configuration is given (e.g. the push
refspecs in remote.*.push, and branch.*.push).

> - current: push "$(HEAD)".  No restrictions on destination.

This updates the branch with the same name the current branch on the
pushing side.

> - matching: push ":" to the destination specified by the current
> branch.

This updates the branches in the destination repository, for which
branches with the same name exists on the pushing side.

> - upstream: In the special case when fetch source is equal to push
> destination, push "$(HEAD):$(branch.$(HEAD).merge)".  Otherwise,
> fallback to current.  Useful in central workflows.

That looks to me as an inconsistent description.  If you are not
pushing to where you fetched from, that is not even central.

This is mean to update the branch that is fetched from and is
integrated with the current branch with the tip of the current
branch, so the branch at the destination repository that gets
updated is branch.$current.merge.  It further means that the
repository being pushed to must be the same as the repository we
fetch from; otherwise it is an error.

> - simple: [still haven't thought about what to do with this; I'm
> generally not in favor of artificially crippling functionality by
> erroring out]

In a central workflow (i.e. repository we fetch from to update the
current branch is the same as the repository we push the tip of this
branch to), this works the same as upstream, but the configured
branch.$current.merge has to be the same as the name of the current
branch in the local repository; otherwise it is an error.

In a triangular workflow TBD, but I think doing the same as current
may be a good starting point.

> Just like upstream respects branch.<name>.merge, current respects
> branch.<name>.push, making branch-level ref mapping in triangular
> workflows possible.

I do not know we want to make branch.*.push linked to current.  If
it is set, shouldn't that apply when push.default is "matching" and
other values?  That is why I threw it in the same category as the
traditional push refspecs in remote.*.push in the early part of this
message.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> [...]
>>
>> Okay, so what you're saying makes sense.
>
> That is not a very useful style of quoting; what did you just agree to?

Ahh.

I took your response as "I'm still resisting [to what was quoted
before it].  But to the part [...] that I am not quoting I would
agree", hence my question.

Did you mean "I'm still resisting, but after reading [...] I think
it makes sense"?  If so, please discard my question.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Did you mean "I'm still resisting, but after reading [...] I think
> it makes sense"?  If so, please discard my question.

Sorry about the lack of clarity.  I agreed with most of what you said,
and I outlined how we could possibly turn it into an implementation.
Still haven't found a solution.

[PATCH 3/4] t/push-default: generalize test_push_{success, commit}

[Ramkumar Ramachandra]

The setup creates two bare repositories: repo1 and repo2, but
test_push_commit() hard-codes checking in repo1 for the actual output.
Generalize it and its caller, test_push_success(), to optionally accept
a third argument to specify the name of the repository to check for
actual output.  We will use this in the next patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5528-push-default.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 4e4824e..9ddd3a9 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -15,17 +15,19 @@ test_expect_success 'setup bare remotes' '
 
 # $1 = local revision
 # $2 = remote revision (tested to be equal to the local one)
+# $3 = [optional] repo to check for actual output (repo1 by default)
 check_pushed_commit () {
 	git log -1 --format='%h %s' "$1" >expect &&
-	git --git-dir=repo1 log -1 --format='%h %s' "$2" >actual &&
+	git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
 	test_cmp expect actual
 }
 
 # $1 = push.default value
 # $2 = expected target branch for the push
+# $3 = [optional] repo to check for actual output (repo1 by default)
 test_push_success () {
 	git -c push.default="$1" push &&
-	check_pushed_commit HEAD "$2"
+	check_pushed_commit HEAD "$2" "$3"
 }
 
 # $1 = push.default value
-- 
1.8.3.247.g485169c

[PATCH 4/4] t/push-default: test pushdefault with all modes

[Ramkumar Ramachandra]

Introduce test_pushdefault_with_mode() to test that remote.pushdefault
works with all the push.default modes correctly.  Exercise it with all
four modes to illustrate how triangular workflows work, and to guard
against regressions.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5528-push-default.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 9ddd3a9..9762515 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -39,6 +39,19 @@ test_push_failure () {
 	test_cmp expect actual
 }
 
+# $1 = push.default value
+# $2 = branch.master.merge value (master or foo)
+# $3 = branch to check for actual output (master or foo)
+test_pushdefault_with_mode () {
+test_expect_success "push.default = $1 works with remote.pushdefault" "
+	test_config branch.master.remote parent1 &&
+	test_config branch.master.merge refs/heads/$2 &&
+	test_config remote.pushdefault parent2 &&
+	test_commit commit-for-$1 &&
+	test_push_success $1 $3 repo2
+"
+}
+
 test_expect_success '"upstream" pushes to configured upstream' '
 	git checkout master &&
 	test_config branch.master.remote parent1 &&
@@ -115,4 +128,9 @@ test_expect_success 'push to existing branch, upstream configured with different
 	test_cmp expect-other-name actual-other-name
 '
 
+test_pushdefault_with_mode "matching" "foo" "master"
+test_pushdefault_with_mode "upstream" "foo" "foo"
+test_pushdefault_with_mode "simple" "master" "master"
+test_pushdefault_with_mode "current" "foo" "master"
+
 test_done
-- 
1.8.3.247.g485169c