[PATCH 0/6] push.default in the triangular world

[PATCH 0/6] push.default in the triangular world

[Ramkumar Ramachandra]

[2/6] documents existing push.default modes properly, but doesn't
touch `simple`.  It incorporates feedback from Junio, Philip Oakley,
Matthieu Moy.

[3/6] gives `simple` an exciting new meaning.  I think it's an
absolutely fabulous default!  It's aimed at triangular people who
occassionally need to do central stuff.

[6/6] adds some new tests to illustrate how all these push.default
modes work in central and triangular workflows.

Thanks.

Ramkumar Ramachandra (6):
  t/t5528-push-default: remove redundant test_config lines
  config doc: rewrite push.default section
  push: change `simple` to accommodate triangular workflows
  push: remove dead code in setup_push_upstream()
  t/t5528-push-default: generalize test_push_*
  t/t5528-push-default: test pushdefault workflows

 Documentation/config.txt | 62 ++++++++++++++++++++++++++----------------------
 builtin/push.c           | 27 +++++++++++++++++----
 t/t5528-push-default.sh  | 46 +++++++++++++++++++++++++++++++----
 3 files changed, 97 insertions(+), 38 deletions(-)

-- 
1.8.3.1.454.g30263f3.dirty

[PATCH 1/6] t/t5528-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.1.454.g30263f3.dirty

Re: [PATCH 1/6] t/t5528-push-default: remove redundant test_config lines

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Makes sense, but I wonder if the only remaining test_config should
also be eradicated in the test after this one.

    test_expect_success '"upstream" does not push 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
    '

It uses test_must_fail but that is only because test_push_failure is
not equipped to handle anything other than "git push" without
arguments.  If the helper were updated to do something like this:

	strategy=$1; shift
	...
	test_must_fail git -c push.default="$strategy" push "$@"

then "test_config push.default" here can be removed and all the
tests will end with test_push_success or test_push_failure.

[PATCH 2/6] config doc: rewrite push.default section

[Ramkumar Ramachandra]

4d3592 (Merge branch 'rr/triangle', 2013-04-07) introduced support for
triangular workflows in Git, but the push.default values still assume
central workflows.  Rewrite the descriptions of `nothing`, `current`,
`upstream` and `matching` for greater clarity, and explicitly explaining
how they behave in triangular workflows.  Leave `simple` as it is for
the moment, as we plan to change its meaning to accommodate triangular
workflows in a later patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 56 ++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7fd4035..9f04f74 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1826,39 +1826,43 @@ 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. Possible values are:
+	Defines the action `git push` should take if no refspec is
+	explicitly given.  Different values are well-suited for
+	specific workflows; for instance, in a purely central workflow
+	(i.e. the fetch source is equal to the push destination),
+	`upstream` is probably what you want.  Possible values are:
 +
 --
-* `nothing` - do not push anything.
-* `matching` - push all branches having the same name in both ends.
-  This is for those who prepare all the branches into a publishable
-  shape and then push them out with a single command.  It is not
-  appropriate for pushing into a repository shared by multiple users,
-  since locally stalled branches will attempt a non-fast forward push
-  if other users updated the branch.
-  +
-  This is currently the default, but Git 2.0 will change the default
-  to `simple`.
-* `upstream` - push the current branch to its upstream branch
-  (`tracking` is a deprecated synonym for this).
-  With this, `git push` will update the same remote ref as the one which
-  is merged by `git pull`, making `push` and `pull` symmetrical.
-  See "branch.<name>.merge" for how to configure the upstream branch.
+* `nothing` - do not push anything (error out) unless a refspec is
+  explicitly given.  Very safe, but not very convenient.
+
+* `current` - push the current branch to update a branch with the same
+  name on the receiving end.  Works in both central and non-central
+  workflows.  Equivalent to pushing the refspec "$branch" ($branch is
+  the name of the current branch).
+
+* `upstream` - push the current branch to a branch with the name
+  branch.$branch.merge on the receiving end, and error out if the push
+  destination is not the same as branch.$branch.remote.  The name
+  "upstream" refers to "@{u[pstream]}" in linkgit:gitrevisions[7],
+  which makes sense only if both branch.$branch.remote and
+  branch.$branch.merge are set.  It makes sure that a `push` is
+  symmetrical to `pull` in central workflows, and cannot be used in
+  non-central workflows.
+
 * `simple` - like `upstream`, but refuses to push if the upstream
   branch's name is different from the local one. This is the safest
   option and is well-suited for beginners. It will become the default
   in Git 2.0.
-* `current` - push the current branch to a branch of the same name.
+
+* `matching` - push all branches having the same name on both ends
+  (essentially ignoring all newly created local branches).
+  Well-suited for those who want to batch-update a specific set of
+  branches they consistently work on.  Use with caution, especially
+  when pushing with '--force'.  Equivalent to pushing the refspec ":".
+  This is currently the default, but Git 2.0 will change the default
+  to `simple`.
 --
-+
-The `simple`, `current` and `upstream` modes are for those who want to
-push out a single branch after finishing work, even when the other
-branches are not yet ready to be pushed out. If you are working with
-other people to push into the same shared repository, you would want
-to use one of these.
 
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
-- 
1.8.3.1.454.g30263f3.dirty

Re: [PATCH 2/6] config doc: rewrite push.default section

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>  push.default::
> +	Defines the action `git push` should take if no refspec is
> +	explicitly given.  Different values are well-suited for
> +	specific workflows; for instance, in a purely central workflow
> +	(i.e. the fetch source is equal to the push destination),
> +	`upstream` is probably what you want.  Possible values are:
>  +
>  --
> -* `nothing` - do not push anything.
> -* `matching` - push all branches having the same name in both ends.
> -  This is for those who prepare all the branches into a publishable
> -  shape and then push them out with a single command.  It is not
> -  appropriate for pushing into a repository shared by multiple users,
> -  since locally stalled branches will attempt a non-fast forward push
> -  if other users updated the branch.
> -  +
> -  This is currently the default, but Git 2.0 will change the default
> -  to `simple`.
> -* `upstream` - push the current branch to its upstream branch
> -  (`tracking` is a deprecated synonym for this).
> -  With this, `git push` will update the same remote ref as the one which
> -  is merged by `git pull`, making `push` and `pull` symmetrical.
> -  See "branch.<name>.merge" for how to configure the upstream branch.
> +* `nothing` - do not push anything (error out) unless a refspec is
> +  explicitly given.  Very safe, but not very convenient.

Drop the last one, that does not add any value to the description.

> +
> +* `current` - push the current branch to update a branch with the same
> +  name on the receiving end.  Works in both central and non-central
> +  workflows.  Equivalent to pushing the refspec "$branch" ($branch is
> +  the name of the current branch).

I do not think this is a place to teach how various refspecs are
spelled, but if you want to, then giving a complete command line
would be more appropriate.  Otherwise I'd suggest dropping
everything after "Equivalent to...".

> +* `upstream` - push the current branch to a branch with the name
> +  branch.$branch.merge on the receiving end, and error out if the push
> +  destination is not the same as branch.$branch.remote.

While the above may be technically accurate, a mechanical definition
help readers who read this for the first time may not be very
helpful.  What does the operation mean?

	push the current branch back to the branch whose changes are
	usually integrated into the current branch with "git pull"
	(which is called @{upstream}).

	this mode only makes sense if you are pushing to the same
	repository you would normally "pull" from (i.e. central
	workflow).

>  * `simple` - like `upstream`, but refuses to push if the upstream
>    branch's name is different from the local one. This is the safest
>    option and is well-suited for beginners. It will become the default
>    in Git 2.0.

OK.

> +* `matching` - push all branches having the same name on both ends
> +  (essentially ignoring all newly created local branches).

That misses the 'essense' of 'matching' by a wide margin, as "newly
created" does not have anything to do with it.  Topic branches you
decided not to push long time ago and haven't pushed out so far will
not suddenly get pushed with "git push", either.

Think of it this way.  Among your local branches, there are classes
of branches that:

 (1) you would want to push now,
 (2) you would want to push when they are done, and
 (3) you would never want to push.

Other "push only single branch" modes make this distinction by
limiting the push to only the first class (those you would want to
push now), but it further limits that set to a single branch, which
is the current one.  It is up to you to differentiate class 2+3
(those you do not want to push now) from class 1 (the single one you
do want to push) and you indicate that choice by running 'git push'
only when you are on the branch you want to push out.

The 'matching' mode allows you to "configure" by having the other
side remember which ones are in the class 1 (i.e. those that you
will push out when you say 'git push').  Instead of keeping a list
of branches to be pushed in local configuration variable, you use
the set of branches your publishing point has.  It has a limitation
that you are not allowed to have the second category (i.e. at this
point of running 'git push' they are not ready to be pushed out),
though.

Perhaps

	... on both ends.  This makes the repository you are pushing
	to remember the set of branches that will be pushed out
	(e.g. if you always push 'maint' and 'master' there and no
	other branches, the repository you push to will have these
	two branches, and your local 'maint' and 'master' will be
	pushed there).

        To use this mode effectively, you have to make sure _all_
	the branches you would push out are ready to be pushed out
	before running 'git push', as the whole point of this mode
	is to allow you to push all of the branches in one go.  If
	you usually finish work on only one branch and push out the
	result, while other branches are unfinished, this mode is
	not for you.  Also this mode is not suitable for pushing
	into a shared central repository, as other people may add
	new branches there, or update the tip of existing branches
	outside your control.

or something like that.

Re: [PATCH 2/6] config doc: rewrite push.default section

[Junio C Hamano]

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

> or something like that.

Just for a completeness, in a patch form:

 Documentation/config.txt | 66 ++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 81628e8..4b7fc54 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1833,37 +1833,49 @@ push.default::
 	`upstream` is probably what you want.  Possible values are:
 +
 --
+
 * `nothing` - do not push anything (error out) unless a refspec is
-  explicitly given.  Very safe, but not very convenient.
+  explicitly given. This is primarily meant for people who want to
+  avoid mistakes by always being explicit.
 
 * `current` - push the current branch to update a branch with the same
   name on the receiving end.  Works in both central and non-central
-  workflows.  Equivalent to pushing the refspec "$branch" ($branch is
-  the name of the current branch).
-
-* `upstream` - push the current branch to a branch with the name
-  branch.$branch.merge on the receiving end, and error out if the push
-  destination is not the same as branch.$branch.remote.  The name
-  "upstream" refers to "@{u[pstream]}" in linkgit:gitrevisions[7],
-  which makes sense only if both branch.$branch.remote and
-  branch.$branch.merge are set.  It makes sure that a `push` is
-  symmetrical to `pull` in central workflows, and cannot be used in
-  non-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, to make sure
-  that a `push` and `push` are never asymmetrical.  It will become the
-  default in Git 2.0.
-
-* `matching` - push all branches having the same name on both ends
-  (essentially ignoring all newly created local branches).
-  Well-suited for those who want to batch-update a specific set of
-  branches they consistently work on.  Use with caution, especially
-  when pushing with '--force'.  Equivalent to pushing the refspec ":".
-  This is currently the default, but Git 2.0 will change the default
-  to `simple`.
+  workflows.
+
+* `upstream` - push the current branch back to the branch whose
+  changes are usually integrated into the current branch (which is
+  called `@{upstream}`).  This mode only makes sense if you are
+  pushing to the same repository you would normally pull from
+  (i.e. central workflow).
+
+* `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 your current
+  branch is not set to integrate with the branch with the same name,
+  to ensure that a `push` and a `push` are symmetrical.
++
+This mode will become the default in Git 2.0.
+
+* `matching` - push all branches having the same name on both ends.
+  This makes the repository you are pushing to remember the set of
+  branches that will be pushed out (e.g. if you always push 'maint'
+  and 'master' there and no other branches, the repository you push
+  to will have these two branches, and your local 'maint' and
+  'master' will be pushed there).
++
+To use this mode effectively, you have to make sure _all_ the
+branches you would push out are ready to be pushed out before
+running 'git push', as the whole point of this mode is to allow you
+to push all of the branches in one go.  If you usually finish work
+on only one branch and push out the result, while other branches are
+unfinished, this mode is not for you.  Also this mode is not
+suitable for pushing into a shared central repository, as other
+people may add new branches there, or update the tip of existing
+branches outside your control.
++
+This is currently the default, but Git 2.0 will change the default
+to `simple`.
+
 --
 
 rebase.stat::
-- 
1.8.3.1-674-gb27e881

Re: [PATCH 2/6] config doc: rewrite push.default section

[Johan Herland]

On Thu, Jun 20, 2013 at 5:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +* `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 your current
> +  branch is not set to integrate with the branch with the same name,
> +  to ensure that a `push` and a `push` are symmetrical.

to ensure that a `push` and a `pull` are symmetrical.

Otherwise, this looks good to me.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Ramkumar Ramachandra]

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

Reported-by: Leandro Lucarella <leandro.lucarella@sociomantic.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 10 ++++++----
 builtin/push.c           | 21 ++++++++++++++++++++-
 t/t5528-push-default.sh  |  2 +-
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9f04f74..81628e8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1850,10 +1850,12 @@ push.default::
   symmetrical to `pull` in central workflows, and cannot be used in
   non-central workflows.
 
-* `simple` - like `upstream`, but refuses to push if the upstream
-  branch's name is different from the local one. This is the safest
-  option and is well-suited for beginners. It will become the default
-  in Git 2.0.
+* `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, to make sure
+  that a `push` and `push` are never asymmetrical.  It will become the
+  default in Git 2.0.
 
 * `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);
+	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
+		/* No upstream configured */
+		goto end;
+	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) &&
+		strcmp(branch->refname, branch->merge[0]->src))
+		/* Central workflow safety feature */
+		die_push_simple(branch, remote);
+end:
+	add_refspec(branch->name);
+}
+
 static void setup_push_upstream(struct remote *remote, int simple)
 {
 	struct strbuf refspec = STRBUF_INIT;
@@ -188,7 +207,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_upstream(remote, 1);
+		setup_push_simple(remote);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
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
 '
 
-- 
1.8.3.1.454.g30263f3.dirty

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

Provided that we would want to keep the "Push the current one to the
same name but you have to have it set up as your integration source"
safety for central workflow (which I am starting to think we
should), we would want something like this on top of your entire
series, I think.  The behaviour change can be seen in the revert of
one test you made to the test that expects "simple" to fail due to
the safety.

This patch is somewhat minimal in that it does not address other
issues I raised in the review of the series; it only addresses the
"simple must be safe" issue.

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

diff --git a/builtin/push.c b/builtin/push.c
index 783bacf..84c4a90 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,29 +120,11 @@ 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);
-	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
-		/* No upstream configured */
-		goto end;
-	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) &&
-		strcmp(branch->refname, branch->merge[0]->src))
-		/* Central workflow safety feature */
-		die_push_simple(branch, remote);
-end:
-	add_refspec(branch->name);
-}
-
-static void setup_push_upstream(struct remote *remote)
+static void setup_push_upstream(struct remote *remote, struct branch *branch,
+				int triangular)
 {
 	struct strbuf refspec = STRBUF_INIT;
-	struct branch *branch = branch_get(NULL);
+
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
@@ -156,16 +138,29 @@ static void setup_push_upstream(struct remote *remote)
 	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))
+	if (triangular)
 		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 (push_default == PUSH_DEFAULT_SIMPLE) {
+		/* Additional safety */
+		if (strcmp(branch->refname, branch->merge[0]->src))
+			die_push_simple(branch, remote);
+	}
+
 	strbuf_addf(&refspec, "%s:%s", branch->name, branch->merge[0]->src);
 	add_refspec(refspec.buf);
 }
 
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+	add_refspec(branch->name);
+}
+
 static char warn_unspecified_push_default_msg[] =
 N_("push.default is unset; its implicit value is changing in\n"
    "Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
@@ -190,9 +185,16 @@ static void warn_unspecified_push_default_configuration(void)
 	warning("%s\n", _(warn_unspecified_push_default_msg));
 }
 
+static int is_workflow_triagular(struct remote *remote)
+{
+	struct remote *fetch_remote = remote_get(NULL);
+	return (fetch_remote != remote);
+}
+
 static void setup_default_push_refspecs(struct remote *remote)
 {
-	struct branch *branch;
+	struct branch *branch = branch_get(NULL);
+	int triangular = is_workflow_triagular(remote);
 
 	switch (push_default) {
 	default:
@@ -205,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_simple(remote);
+		if (triangular)
+			setup_push_current(remote, branch);
+		else
+			setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote);
+		setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-		branch = branch_get(NULL);
-		if (!branch)
-			die(_(message_detached_head_die), remote->name);
-		add_refspec(branch->name);
+		setup_push_current(remote, branch);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index eabc09d..36f5a63 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -107,7 +107,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_success simple master &&
+	test_push_failure simple &&
 	test_push_failure upstream
 '
 

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> 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?

They're not the same thing.  It is very much intentional and intended:
the safety net is not to "ensure that the push and pull are
symmetrical" (i.e. among other things, error out if
branch.$branch.merge is unset), but rather "ensure that the push and
pull are never asymmetrical".

>> 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.
>
> Provided that we would want to keep the "Push the current one to the
> same name but you have to have it set up as your integration source"
> safety for central workflow (which I am starting to think we
> should), we would want something like this on top of your entire
> series, I think.  The behaviour change can be seen in the revert of
> one test you made to the test that expects "simple" to fail due to
> the safety.

Now I'd like to question what you are labelling as "safety".  What is
the consequence of erroring out when branch.$branch.merge is unset
when pushing using `upstream`?  For me, it only means additional
inconvenience: any new branches I create can't be pushed out without
explicitly setting branch.$branch.merge to an "invalid" value.  What
is invalid about it?  The fact that it doesn't exist, @{u} still
doesn't resolve, and git branch -u doesn't work.  Hell, even git push
-u doesn't work!  So, what is this huge "safety" that can justify
inconveniencing me like this?  By making sure that
branch.$branch.merge is set, my prompt responds immediately to
divergence, and this is awesome.  Predictably, I use git push -u when
I push out a new branch with `current`.  So, unless you have a damn
good reason to inconvenience me in the name of safety,
branch.$branch.merge should default to refs/heads/$branch, unless set
explicitly.

I didn't want to contaminate this series with an unrelated improvement
to `upstream`, which is why you don't see the change here: it is
orthogonal to designing a good `simple`, and I only brought it up to
question the "safety" you're carrying over to `simple`; what
obligation does `simple` have to carry over this "feature"?  I've made
it clear that I want a clean break from `upstream`, and I find your
proposal is very inelegant: `simple` has two modes of operation; when
branch.$branch.remote is equal to $pushremote, branch.$branch.merge
must be set and equal to $branch (the `upstream` mode); when
branch.$branch.remote is unequal to $pushremote, don't care about
whether branch.$branch.merge is set (the `current` mode).  My proposal
is much smoother than this "modal" operation, no?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>> 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?
>
> They're not the same thing.  It is very much intentional and intended:
> the safety net is not to "ensure that the push and pull are
> symmetrical" (i.e. among other things, error out if
> branch.$branch.merge is unset), but rather "ensure that the push and
> pull are never asymmetrical".

Hmmmm....

    not to "ensure that the push and pull are symmetrical"
    rather "ensure that the push and pull are never asymmetrical".

They still talk the same thing to me.  What am I missing?

Am I being clueless, or is there something else going on?

Your "among other things", after reading it three times,
unfortunately did not help clarify anything to me, so perhaps
somebody other than me (or you for that matter) who is more clueful
can help find a different way to explain the difference you are
trying to express to me.

Help, anybody?

>> Provided that we would want to keep the "Push the current one to the
>> same name but you have to have it set up as your integration source"
>> safety for central workflow (which I am starting to think we
>> should), we would want something like this on top of your entire
>> series, I think.  The behaviour change can be seen in the revert of
>> one test you made to the test that expects "simple" to fail due to
>> the safety.
>
> Now I'd like to question what you are labelling as "safety".  What is
> the consequence of erroring out when branch.$branch.merge is unset
> when pushing using `upstream`?

Nothing noteworthy.

I wasn't suggesting to change what `upstream` does at all.

If you do not have a `upstream` configured, we do not know what
branch you are integrating with, and the operation has failed in the
code even before we started adding triangular.

I do not see a reason to change that in the triangular world;
`upstream` is about the central workflow as you originally wrote in
the "config.txt" patch in this series.

The name of the branch the repository you fetch from and integrate
with does not have anything to do with the name you want to push
your derived work as to a different repository

I thought we already discussed and agreed on this point.

  http://thread.gmane.org/gmane.comp.version-control.git/227028/focus=227313

The conclusion is that using push.default=`upstream` is meaningless
when you are using a triangular workflow.  If you are using a
centralized workflow, and if a branch does not have branch.*.merge
configured, we do not know to which branch you are pushing it back,
so we error out.

What I am changing from the patch you posted with the "how about
this on top" patch back to the current behaviour is what 'simple'
does for centralized workflow.

> I didn't want to contaminate this series with an unrelated improvement
> to `upstream`

I think we share that, and it is not just `upstream`, but also
`simple` when it is applied to folks who employ a centralized
workflow.  The safety we need to keep is the one we have had since
day one of introducing 'simple' for them.

When you are doing a centralized workflow, 'simple' was defined to
be 'upstream' with added restriction that the branch at the remote
you integrate with must have the same name as the current branch you
are pushing, i.e.  in

    [branch "frotz"]
	merge = refs/heads/$branch

the value of $branch must be 'frotz'; otherwise 'simple' errors out.

  http://thread.gmane.org/gmane.comp.version-control.git/194175/focus=196199

Now, no existing series has casted in stone the best behaviour for
`simple` in a triangular workflow.  With this series (and also with
my fixup patch I sent last night), it is defined to act as `current`,
but it may need a bit more safety to help new users avoid pushing
branches they did not intend to (perhaps pushing out `current` only
when the branch with the same name already exists at the destination?
I dunno).

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

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Thursday, June 20, 2013 8:23 PM
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>>> 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?
>>
>> They're not the same thing.  It is very much intentional and 
>> intended:
>> the safety net is not to "ensure that the push and pull are
>> symmetrical" (i.e. among other things, error out if
>> branch.$branch.merge is unset), but rather "ensure that the push and
>> pull are never asymmetrical".
>
> Hmmmm....
>
>    not to "ensure that the push and pull are symmetrical"
>    rather "ensure that the push and pull are never asymmetrical".
>
> They still talk the same thing to me.  What am I missing?
>
> Am I being clueless, or is there something else going on?

I think it is a case of the user having explicitly set push=Africa and 
pull=Europe which can't be a setting for simple symmetry.
But then again I haven't been following the fine details. (that is there 
are some defaults that allow stuff to be half set)

>
> Your "among other things", after reading it three times,
> unfortunately did not help clarify anything to me, so perhaps
> somebody other than me (or you for that matter) who is more clueful
> can help find a different way to explain the difference you are
> trying to express to me.
>
> Help, anybody?
>

Philip

[...] 

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

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Thursday, June 20, 2013 8:23 PM
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>> Junio C Hamano wrote:
>>>> Double negation confused my parser.  'push' and 'pull' should be
>>>> kept symmetrical in central workflows?
>>>
>>> They're not the same thing.  It is very much intentional and
>>> intended:
>>> the safety net is not to "ensure that the push and pull are
>>> symmetrical" (i.e. among other things, error out if
>>> branch.$branch.merge is unset), but rather "ensure that the push and
>>> pull are never asymmetrical".
>>
>> Hmmmm....
>>
>>    not to "ensure that the push and pull are symmetrical"
>>    rather "ensure that the push and pull are never asymmetrical".
>>
>> They still talk the same thing to me.  What am I missing?
>>
>> Am I being clueless, or is there something else going on?
>
> I think it is a case of the user having explicitly set push=Africa and
> pull=Europe which can't be a setting for simple symmetry.

Yeah but then that is not a discussion about central workflow.

I can understand "In a central workflow push and pull should be
symmetrical."  I can also, with a bit of double-negation brain
twisting, understand "In a central workflow, push and pull should
not be asymmetrical."

But when I suggest to avoid double-negation, I was told that these
two statements mean different things, and the original should not be
rewritten to avoid double-negation, which is where my brain stopped
and asked for help.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> They're not the same thing.  It is very much intentional and intended:
>> the safety net is not to "ensure that the push and pull are
>> symmetrical" (i.e. among other things, error out if
>> branch.$branch.merge is unset), but rather "ensure that the push and
>> pull are never asymmetrical".
>
>     not to "ensure that the push and pull are symmetrical"
>     rather "ensure that the push and pull are never asymmetrical".
>
> They still talk the same thing to me.  What am I missing?

Never mind the wording then.  I am proposing preventing asymmetry by
explicitly disallowing a push when $branch is different from
branch.$branch.merge, instead of shutting down immediately when
branch.$branch.merge is unset.

>> Now I'd like to question what you are labelling as "safety".  What is
>> the consequence of erroring out when branch.$branch.merge is unset
>> when pushing using `upstream`?
>
> Nothing noteworthy.
>
> I wasn't suggesting to change what `upstream` does at all.

No, but I did.  I just argued for a sane default for
branch.$branch.merge (the part you snipped out).

> The conclusion is that using push.default=`upstream` is meaningless
> when you are using a triangular workflow.

Yes, and I agreed with that.

> If you are using a
> centralized workflow, and if a branch does not have branch.*.merge
> configured, we do not know to which branch you are pushing it back,
> so we error out.

And I said: have a sane default.

> What I am changing from the patch you posted with the "how about
> this on top" patch back to the current behaviour is what 'simple'
> does for centralized workflow.

Yes, I know.  I read the patch.

> When you are doing a centralized workflow, 'simple' was defined

Again, I'm well aware what it _was_ defined as.  Was it not clear that
I argued for a change from the very first email where I posted the
patch and changed a test?  Do you have a counter-argument, or is it
simply that we must respect its historical meaning?

> Now, no existing series has casted in stone the best behaviour for
> `simple` in a triangular workflow.  With this series (and also with
> my fixup patch I sent last night), it is defined to act as `current`,
> but it may need a bit more safety to help new users avoid pushing
> branches they did not intend to (perhaps pushing out `current` only
> when the branch with the same name already exists at the destination?
> I dunno).

I see no reason to plan safety features in advance, especially since
we have neither branch.$branch.push nor @{push} yet.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>> They're not the same thing.  It is very much intentional and intended:
>>> the safety net is not to "ensure that the push and pull are
>>> symmetrical" (i.e. among other things, error out if
>>> branch.$branch.merge is unset), but rather "ensure that the push and
>>> pull are never asymmetrical".
>>
>>     not to "ensure that the push and pull are symmetrical"
>>     rather "ensure that the push and pull are never asymmetrical".
>>
>> They still talk the same thing to me.  What am I missing?
>
> Never mind the wording then.  I am proposing preventing asymmetry by
> explicitly disallowing a push when $branch is different from
> branch.$branch.merge, instead of shutting down immediately when
> branch.$branch.merge is unset.

We always said "upstream is to update the branch you fetch and
integrate with", and tried to make sure the push destination is the
branch you configured the current branch (i.e. the one you are
trying to push out) to fetch and integrate with.  That is how we
prevent asymmetry.

We fail if branch.$branch.merge is set to something else.  We also
fail if branch.$branch.merge is *not* set, because by definition the
branch you are trying to push to in that scenario cannot be the
branch you fetch and integrat with by "git pull [--rebase]".

I know your patch was attempting to change the behaviour for the
latter.  You are trying to let anything go if branch.*.merge is not
set.  How would it help prevent assymetry?

>>> Now I'd like to question what you are labelling as "safety".  What is
>>> the consequence of erroring out when branch.$branch.merge is unset
>>> when pushing using `upstream`?
>>
>> Nothing noteworthy.
>>
>> I wasn't suggesting to change what `upstream` does at all.
>
> No, but I did....

Really?  Then where did this come from?

> I didn't want to contaminate this series with an unrelated improvement
> to `upstream`

>> If you are using a
>> centralized workflow, and if a branch does not have branch.*.merge
>> configured, we do not know to which branch you are pushing it back,
>> so we error out.
>
> And I said: have a sane default.

Like you said, I do not want to contaminate this series with such an
unrelated change.  Worse, you are trying to break a sane default by
replacing it with "anything goes".

We already have a sane default, which is to error out.  We do not
need your broken default.

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

[Junio C Hamano]

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

> Like you said, I do not want to contaminate this series with such an
> unrelated change.  Worse, you are trying to break a sane default by
> replacing it with "anything goes".
>
> We already have a sane default, which is to error out.  We do not
> need your broken default.

This came out as a bit stronger than I wanted to.  Add to the last:

    Even if we later find out that changing the default to loosen it
    to "anything goes" is a good idea, I do not think it belongs to
    this series.

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

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> Provided that we would want to keep the "Push the current one to the
> same name but you have to have it set up as your integration source"
> safety for central workflow (which I am starting to think we
> should), we would want something like this on top of your entire
> series, I think.  The behaviour change can be seen in the revert of
> one test you made to the test that expects "simple" to fail due to
> the safety.

And with the small refactoring of setup_default_push_refspecs (the
important part being that we grab branch in this function, not in
its helper functions per push.default mode), branch.*.push can fall
out rather naturally, like this patch on top.


A footnote unrelated to this patch.

The function is_workflow_triangular() in the "how about this" patch
needs to be tweaked from the version I am responding to, in order to
take the case where fetch-remote is not defined into account, i.e.

    static int is_workflow_triagular(struct remote *remote)
    {
            struct remote *fetch_remote = remote_get(NULL);
            return (fetch_remote && fetch_remote != remote);
    }




 builtin/push.c | 18 +++++++++++++++++-
 remote.c       |  5 +++++
 remote.h       |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index f6c8047..a140b8e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -185,6 +185,15 @@ static void warn_unspecified_push_default_configuration(void)
 	warning("%s\n", _(warn_unspecified_push_default_msg));
 }
 
+static void setup_per_branch_push(struct branch *branch)
+{
+	struct strbuf refspec = STRBUF_INIT;
+
+	strbuf_addf(&refspec, "%s:%s",
+		    branch->name, branch->push_name);
+	add_refspec(refspec.buf);
+}
+
 static int is_workflow_triagular(struct remote *remote)
 {
 	struct remote *fetch_remote = remote_get(NULL);
@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
 static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch = branch_get(NULL);
-	int triangular = is_workflow_triagular(remote);
+	int triangular;
+
+	if (branch->push_name) {
+		setup_per_branch_push(branch);
+		return;
+	}
+
+	triangular = is_workflow_triagular(remote);
 
 	switch (push_default) {
 	default:
diff --git a/remote.c b/remote.c
index e71f66d..e033fef 100644
--- a/remote.c
+++ b/remote.c
@@ -372,6 +372,11 @@ static int handle_config(const char *key, const char *value, void *cb)
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
+		} else if (!strcmp(subkey, ".push")) {
+			if (!value)
+				return config_error_nonbool(key);
+			free(branch->push_name);
+			branch->push_name = xstrdup(value);
 		}
 		return 0;
 	}
diff --git a/remote.h b/remote.h
index cf56724..84e0f72 100644
--- a/remote.h
+++ b/remote.h
@@ -138,6 +138,8 @@ struct branch {
 	struct refspec **merge;
 	int merge_nr;
 	int merge_alloc;
+
+	char *push_name;
 };
 
 struct branch *branch_get(const char *name);

[PATCH 4/6] push: remove dead code in setup_push_upstream()

[Ramkumar Ramachandra]

Now that simple has been decoupled from upstream in setup_push_simple(),
remove the dead code in setup_push_upstream().

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

diff --git a/builtin/push.c b/builtin/push.c
index d8d27d9..783bacf 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -139,7 +139,7 @@ end:
 	add_refspec(branch->name);
 }
 
-static void setup_push_upstream(struct remote *remote, int simple)
+static void setup_push_upstream(struct remote *remote)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
@@ -161,8 +161,6 @@ static void setup_push_upstream(struct remote *remote, int simple)
 		      "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);
 
 	strbuf_addf(&refspec, "%s:%s", branch->name, branch->merge[0]->src);
 	add_refspec(refspec.buf);
@@ -211,7 +209,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, 0);
+		setup_push_upstream(remote);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-- 
1.8.3.1.454.g30263f3.dirty

Re: [PATCH 4/6] push: remove dead code in setup_push_upstream()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Now that simple has been decoupled from upstream in setup_push_simple(),
> remove the dead code in setup_push_upstream().

Good.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/push.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index d8d27d9..783bacf 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -139,7 +139,7 @@ end:
>  	add_refspec(branch->name);
>  }
>  
> -static void setup_push_upstream(struct remote *remote, int simple)
> +static void setup_push_upstream(struct remote *remote)
>  {
>  	struct strbuf refspec = STRBUF_INIT;
>  	struct branch *branch = branch_get(NULL);
> @@ -161,8 +161,6 @@ static void setup_push_upstream(struct remote *remote, int simple)
>  		      "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);
>  
>  	strbuf_addf(&refspec, "%s:%s", branch->name, branch->merge[0]->src);
>  	add_refspec(refspec.buf);
> @@ -211,7 +209,7 @@ static void setup_default_push_refspecs(struct remote *remote)
>  		break;
>  
>  	case PUSH_DEFAULT_UPSTREAM:
> -		setup_push_upstream(remote, 0);
> +		setup_push_upstream(remote);
>  		break;
>  
>  	case PUSH_DEFAULT_CURRENT:

[PATCH 5/6] t/t5528-push-default: generalize test_push_*

[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 e54dd02..b599186 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.1.454.g30263f3.dirty

Re: [PATCH 5/6] t/t5528-push-default: generalize test_push_*

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Makes sense.

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

[PATCH 6/6] t/t5528-push-default: test pushdefault workflows

[Ramkumar Ramachandra]

Introduce test_pushdefault_workflows(), and test that all push.default
modes work with central and triangular workflows as expected.

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

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index b599186..eabc09d 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -39,6 +39,26 @@ test_push_failure () {
 	test_cmp expect actual
 }
 
+# $1 = success or failure
+# $2 = push.default value
+# $3 = branch to check for actual output (master or foo)
+# $4 = [optional] switch to triangular workflow
+test_pushdefault_workflow () {
+	workflow=central
+	pushdefault=parent1
+	if test -n "${4-}"; then
+		workflow=triangular
+		pushdefault=parent2
+	fi
+test_expect_success "push.default = $2 $1 in $workflow workflows" "
+	test_config branch.master.remote parent1 &&
+	test_config branch.master.merge refs/heads/foo &&
+	test_config remote.pushdefault $pushdefault &&
+	test_commit commit-for-$2${4+-triangular} &&
+	test_push_$1 $2 $3 ${4+repo2}
+"
+}
+
 test_expect_success '"upstream" pushes to configured upstream' '
 	git checkout master &&
 	test_config branch.master.remote parent1 &&
@@ -115,4 +135,20 @@ test_expect_success 'push to existing branch, upstream configured with different
 	test_cmp expect-other-name actual-other-name
 '
 
+## test_pushdefault_workflow() arguments:
+# $1 = success or failure
+# $2 = push.default value
+# $3 = branch to check for actual output (master or foo)
+# $4 = [optional] switch to triangular workflow
+
+test_pushdefault_workflow success current master  # breaks push/pull symmetry
+test_pushdefault_workflow success upstream foo    # preserves push/pull symmetry
+test_pushdefault_workflow failure simple master   # errors out on asymmetry
+test_pushdefault_workflow success matching master # always works
+
+test_pushdefault_workflow success current master triangular  # always works
+test_pushdefault_workflow failure upstream foo triangular    # always errors out
+test_pushdefault_workflow success simple master triangular   # works like current
+test_pushdefault_workflow success matching master triangular # always works
+
 test_done
-- 
1.8.3.1.454.g30263f3.dirty

Re: [PATCH 6/6] t/t5528-push-default: test pushdefault workflows

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Introduce test_pushdefault_workflows(), and test that all push.default
> modes work with central and triangular workflows as expected.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5528-push-default.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index b599186..eabc09d 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -39,6 +39,26 @@ test_push_failure () {
>  	test_cmp expect actual
>  }
>  
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow
> +test_pushdefault_workflow () {
> +	workflow=central
> +	pushdefault=parent1
> +	if test -n "${4-}"; then
> +		workflow=triangular
> +		pushdefault=parent2
> +	fi
> +test_expect_success "push.default = $2 $1 in $workflow workflows" "
> +	test_config branch.master.remote parent1 &&
> +	test_config branch.master.merge refs/heads/foo &&
> +	test_config remote.pushdefault $pushdefault &&
> +	test_commit commit-for-$2${4+-triangular} &&
> +	test_push_$1 $2 $3 ${4+repo2}
> +"

Please indent the above; it is hard to spot where the actual test
begins and ends otherwise.

> +}
> +
>  test_expect_success '"upstream" pushes to configured upstream' '
>  	git checkout master &&
>  	test_config branch.master.remote parent1 &&
> @@ -115,4 +135,20 @@ test_expect_success 'push to existing branch, upstream configured with different
>  	test_cmp expect-other-name actual-other-name
>  '
>  
> +## test_pushdefault_workflow() arguments:
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow

What happens in current/upstream/simple modes is affected by the
current branch, but it is unclear on what branch do these all run at
the first glance.  It would be helpful to add a comment here that
says what the starting condition is, something like:

    # We are on 'master', which integrates with 'foo' from parent1
    # Both parent1 and parent2 repositories have 'master' and 'foo'
    # branches.

here.  It took me a while to tell what some of these comments wanted
to say without such a comment.

> +test_pushdefault_workflow success current master  # breaks push/pull symmetry

On 'master', push.default = current and we are in central workflow.
Because we do not use upstream but current, that deliberately breaks
the symmetry.  We make sure that 'master' is updated.

Looks correct.

> +test_pushdefault_workflow success upstream foo    # preserves push/pull symmetry

We use 'upstream'; current branch is set to integrate with 'foo' and
that is what is updated.  Looks good.

> +test_pushdefault_workflow failure simple master   # errors out on asymmetry

Simple is a safer form of 'current' in the central workflow.
The current branch is set to integrate with 'foo', which is
different name from the current branch'es name 'master', and the
push fails.

Looks correct, but do we want to make sure 'foo' is not updated
here as well?

> +test_pushdefault_workflow success matching master # always works

This also should update 'foo'; do we want to make sure that happens
too?

Otherwise we won't be able to tell between matching and current.

> +test_pushdefault_workflow success current master triangular  # always works

OK.

> +test_pushdefault_workflow failure upstream foo triangular    # always errors out

OK.

> +test_pushdefault_workflow success simple master triangular   # works like current

OK, because in triangular it is like current.

> +test_pushdefault_workflow success matching master triangular # always works

OK.

As this step adds a useful helper function for testing, it appears
to me that the helper can and should check the postcondition more
carefully, e.g. not just making sure what should be updated is
updated, but what should not be updated is not touched.

Other than that, looks fine to me.

> +
>  test_done