[PATCH 0/6] Reroll of rr/triangular-push-fix

[PATCH 0/6] Reroll of rr/triangular-push-fix

[Junio C Hamano]

This is mostly Ram's triangular-push-fix series, but the fix-up
commits I queued on top of it when the series was queued to 'pu'
have been squashed in.

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

  No changes, as posted by Ram.

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

  Reorganization (including moving of 'matching' to the end) is Ram's,
  but otherwise moderately rewritten.

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

  Sqaushed in the fix to keep the semantics of "simple" when used in
  the centralized workflow the same as before.

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

  As posted by Ram.

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

  A style fix from the review, and comment tweaks to describe what
  each tests mean better.

  [PATCH 6/6] push: honor branch.*.push

  This is new.  It probably needs tests and docs.

I am trying this myself primarily because this changes the plan for
Git 2.0 and jc/push-2.0-default-to-simple topic needs to be redone,
but before we can do so, we need to see this topic solidify enough.

Junio C Hamano (1):
  push: honor branch.*.push

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

 Documentation/config.txt | 80 ++++++++++++++++++++++++++++++------------------
 builtin/push.c           | 59 +++++++++++++++++++++++++++--------
 remote.c                 |  5 +++
 remote.h                 |  2 ++
 t/t5528-push-default.sh  | 65 ++++++++++++++++++++++++++++++++++++---
 5 files changed, 165 insertions(+), 46 deletions(-)

-- 
1.8.3.1-721-g0a353d3

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

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.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-721-g0a353d3

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

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

4d35924e (Merge branch 'rr/triangle', 2013-04-07) introduced support
for triangular workflows, but the push.default values still assume
central workflows.

Rewrite the descriptions of `nothing`, `current`, `upstream` and
`matching` for greater clarity, and explicitly explain how they
should 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 72 +++++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7fd4035..5d8ff1a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1826,39 +1826,55 @@ 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. 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.
+
+* `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` - 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.
---
+  option and is well-suited for beginners.
 +
-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.
+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::
 	Whether to show a diffstat of what changed upstream since the last
-- 
1.8.3.1-721-g0a353d3

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

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

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.

The very name of "upstream" 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.

Redefine "simple" as "safer upstream" for centralized workflow as
before, but work as "current" for triangular workflow.

An earlier round of this change by mistake broke the safety for
"simple" mode we have had since day 1 of that mode to make sure that
the branch in the repository we update is set to be the one we fetch
and integrate with, but it has been fixed.

Reported-by: Leandro Lucarella <leandro.lucarella@sociomantic.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 10 +++++++---
 builtin/push.c           | 43 +++++++++++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5d8ff1a..cae6870 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,9 +1848,13 @@ push.default::
   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.
+* `simple` - in centralized workflow, work like `upstream` with an
+  added safety to refuse to push if the upstream branch's name is
+  different from the local one.
++
+When pushing to a remote that is different from the remote you normally
+pull from, work as `current`.  This is the safest option and is suited
+for beginners.
 +
 This mode will become the default in Git 2.0.
 
diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..f6c8047 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,10 +120,11 @@ static const char message_detached_head_die[] =
 	   "\n"
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
-static void setup_push_upstream(struct remote *remote, int simple)
+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)
@@ -137,18 +138,29 @@ 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))
+	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 (simple && strcmp(branch->refname, branch->merge[0]->src))
-		die_push_simple(branch, remote);
+
+	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"
@@ -173,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 && 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:
@@ -188,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_upstream(remote, 1);
+		if (triangular)
+			setup_push_current(remote, branch);
+		else
+			setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, 0);
+		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:
-- 
1.8.3.1-721-g0a353d3

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

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.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 69ce6bf..db58e7f 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-721-g0a353d3

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

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5528-push-default.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index db58e7f..6a5ac3a 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,41 @@ test_expect_success 'push to existing branch, upstream configured with different
 	test_cmp expect-other-name actual-other-name
 '
 
+# We are on 'master', which integrates with 'foo' from parent1
+# remote (set in test_pushdefault_workflow helper).  Push to
+# parent1 in centralized, and push to parent2 in triangular workflow.
+# The parent1 repository has 'master' and 'foo' branches, while
+# the parent2 repository has only 'master' branch.
+#
+# 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
+
+# update parent1's master (which is not our upstream)
+test_pushdefault_workflow success current master
+
+# update parent1's foo (which is our upstream)
+test_pushdefault_workflow success upstream foo
+
+# upsream is foo which is not the name of the current branch
+test_pushdefault_workflow failure simple master
+
+# master and foo are updated
+test_pushdefault_workflow success matching master
+
+# master is updated
+test_pushdefault_workflow success current master triangular
+
+# upstream mode cannot be used in triangular
+test_pushdefault_workflow failure upstream foo triangular
+
+# in triangular, 'simple' works as 'current' and update the branch
+# with the same name.
+test_pushdefault_workflow success simple master triangular
+
+# master is updated (parent2 does not have foo)
+test_pushdefault_workflow success matching master triangular
+
 test_done
-- 
1.8.3.1-721-g0a353d3

[PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

When branch.*.push configuration variable is defined for the current
branch, a lazy-typing "git push" (and "git push there") will push
the commit at the tip of the current branch to the destination and
update the branch named by that variable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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);
-- 
1.8.3.1-721-g0a353d3

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

[Johan Herland]

On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Ramkumar Ramachandra <artagnon@gmail.com>
>
> 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.
>
> The very name of "upstream" 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.
>
> Redefine "simple" as "safer upstream" for centralized workflow as
> before, but work as "current" for triangular workflow.
>
> An earlier round of this change by mistake broke the safety for
> "simple" mode we have had since day 1 of that mode to make sure that
> the branch in the repository we update is set to be the one we fetch
> and integrate with, but it has been fixed.

Shouldn't there be an acompanying test to demonstrate this mistake being fixed?

> Reported-by: Leandro Lucarella <leandro.lucarella@sociomantic.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config.txt | 10 +++++++---
>  builtin/push.c           | 43 +++++++++++++++++++++++++++++++------------
>  2 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5d8ff1a..cae6870 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1848,9 +1848,13 @@ push.default::
>    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.
> +* `simple` - in centralized workflow, work like `upstream` with an
> +  added safety to refuse to push if the upstream branch's name is
> +  different from the local one.
> ++
> +When pushing to a remote that is different from the remote you normally
> +pull from, work as `current`.  This is the safest option and is suited
> +for beginners.
>  +
>  This mode will become the default in Git 2.0.
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 2d84d10..f6c8047 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -120,10 +120,11 @@ static const char message_detached_head_die[] =
>            "\n"
>            "    git push %s HEAD:<name-of-remote-branch>\n");
>
> -static void setup_push_upstream(struct remote *remote, int simple)
> +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)
> @@ -137,18 +138,29 @@ 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))
> +       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 (simple && strcmp(branch->refname, branch->merge[0]->src))
> -               die_push_simple(branch, remote);
> +
> +       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);

Here (and above) we add a refspec to tell Git exactly what to push
from the local end, and into what on the remote end. Is it possible to
end up with multiple simultaneous refspecs matching the same local
ref, but mapping to different remote refs? If so, which will win, and
does that make sense?

> +}
> +
>  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"
> @@ -173,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)

s/triagular/triangular/

> +{
> +       struct remote *fetch_remote = remote_get(NULL);
> +       return (fetch_remote && fetch_remote != remote);

This changed from a strcmp() to a pointer compare. That might be safe,
depending on the sources of the two struct remote *, but I'm not sure.

> +}
> +
>  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:
> @@ -188,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)
>                 break;
>
>         case PUSH_DEFAULT_SIMPLE:
> -               setup_push_upstream(remote, 1);
> +               if (triangular)
> +                       setup_push_current(remote, branch);
> +               else
> +                       setup_push_upstream(remote, branch, triangular);
>                 break;
>
>         case PUSH_DEFAULT_UPSTREAM:
> -               setup_push_upstream(remote, 0);
> +               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:

Otherwise, this looks good to me.

...Johan

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

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

[Johan Herland]

On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Ramkumar Ramachandra <artagnon@gmail.com>
>
> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.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 69ce6bf..db58e7f 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 &&

Isn't  ${3:-repo1} a bashism?

>         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-721-g0a353d3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH 6/6] push: honor branch.*.push

[Johan Herland]

On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> When branch.*.push configuration variable is defined for the current
> branch, a lazy-typing "git push" (and "git push there") will push
> the commit at the tip of the current branch to the destination and
> update the branch named by that variable.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  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);

This goes back to the question I raised in 3/6: If this code path adds
refspec "foo:bar", and - say - setup_push_current() has already added
refspec "foo:foo" (or simply "foo"), then do we end up pushing into
"foo" or "bar"? To me, "branch.*.push" feels more specific than
"push.default = current", so it would make sense that "foo:bar"
overrides "foo:foo", but that is not obvious from the refspec alone.
IMHO, this definitely needs some tests.

> +}
> +
>  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;

I guess this return ensures that branch.*.push overrides push.default,
but might there be other sources of add_refspec() that would
complicate things?

> +       }
> +
> +       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);

Otherwise, this patch, and the entire series looks good to me.


...Johan

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

Re: [PATCH 0/6] Reroll of rr/triangular-push-fix

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>   [PATCH 3/6] push: change `simple` to accommodate triangular workflows
>
>   Sqaushed in the fix to keep the semantics of "simple" when used in
>   the centralized workflow the same as before.

Yeah, I'm worried about this as I pointed out earlier.  I don't like
erroring out when no branch.$branch.merge is not explicitly set, when
the 95% usecase is not naming local branches differently from remote
branches (oh, and I already pointed out how difficult it is to set the
damn thing).  So, I'm working on a series to  make
branch.$branch.merge default to refs/heads/$branch.  Yes, I'm aware of
your "argument":

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

I hope (perhaps foolishly) to persuade you nevertheless. I fear that
if this series solidifies before I get there, we'll be stuck with this
stupid erroring-out behavior forever.

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> +       git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
>
> Isn't  ${3:-repo1} a bashism?

I do not think so.  But now I looked at it again, I think I would
use ${3-repo1} form in this case myself.  No caller passes an empty
string to the third place.

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> +static void setup_push_current(struct remote *remote, struct branch *branch)
>> +{
>> +       if (!branch)
>> +               die(_(message_detached_head_die), remote->name);
>> +       add_refspec(branch->name);
>
> Here (and above) we add a refspec to tell Git exactly what to push
> from the local end, and into what on the remote end.

Correct.

> Is it possible to end up with multiple simultaneous refspecs
> matching the same local ref, but mapping to different remote refs?

Sorry, I don't follow.  If you say "push.default = current" and you
do not give any other stronger clue (e.g. "git push origin master"
on the command line, or "git push [origin]" with remote.origin.push
configured), the above function is called and sets up your current
branch to be pushed to the same.

It is a bit more interesting for "push.default = upstream", which is
for centralized workflow.  If you forked frotz and nitfol branches
both from their master, e.g.

	$ git checkout -t -b frotz origin/master
	$ git checkout -t -b nitfol origin/master

after having worked on one of the branches, when you want to push it
back, the result of working on the topic branch goes back to master,
but I think that is what you want in the centralized workflow.  If
it fast-forwards, you are fine, and if it does not, you will fetch
your upstream, i.e. their master, integrate your work with it, and
then push it back.  At that point, you are playing the role of the
integrator of the shared master branch, because what you do on your
topic branch when you integrate others' work from master is exactly
that---you are not perfecting the theme you wanted to achieve on
your topic branch, but are integrating that result into shared
master to advance the overall state of the project.  So pushing the
result back to 'master' makes perfect sense.  After that, when you
have to restart your work on the other branch, you may first "pull
--rebase" before continuing, or you may just keep going with your
work based on a tad old origin/master.  But when you finish working
on that topic and are about to push it out, you would be doing the
same "tentatively don the central integrator's hat", and again it
makes sense to push the result to 'master'.

So in that sense, it is not "which one wins".  It is more like "you
can push only after you become up to date, so there isn't one branch
overwriting the other one."

That is how I view it, anyway.

 cf. http://git-blame.blogspot.com/2013/06/fun-with-various-workflows-1.html

>> +static int is_workflow_triagular(struct remote *remote)
>
> s/triagular/triangular/

Thanks.

>
>> +{
>> +       struct remote *fetch_remote = remote_get(NULL);
>> +       return (fetch_remote && fetch_remote != remote);
>
> This changed from a strcmp() to a pointer compare. That might be safe,
> depending on the sources of the two struct remote *, but I'm not sure.

Given the way remote_get() works, it should be correct, I think.

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

[Ramkumar Ramachandra]

Johan Herland wrote:
>> An earlier round of this change by mistake broke the safety for
>> "simple" mode we have had since day 1 of that mode to make sure that
>> the branch in the repository we update is set to be the one we fetch
>> and integrate with, but it has been fixed.
>
> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?

Read "earlier iteration": it didn't get merged.

>> +static void setup_push_current(struct remote *remote, struct branch *branch)
>> +{
>> +       if (!branch)
>> +               die(_(message_detached_head_die), remote->name);
>> +       add_refspec(branch->name);
>
> Here (and above) we add a refspec to tell Git exactly what to push
> from the local end, and into what on the remote end.

Nope, we add the refspec "foo", without the :destination part.  The
remote end is unspecified (and defaults to "foo", but that is in the
transport layer).

> Is it possible to
> end up with multiple simultaneous refspecs matching the same local
> ref, but mapping to different remote refs? If so, which will win, and
> does that make sense?

It is impossible.  We either:

- Get an explicit refspec from the user and never run
setup_default_push_refspecs() to begin with.

- Run setup_push_refspecs() and add *one* refspec depending on the
push.default value.

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> +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);
>
> This goes back to the question I raised in 3/6: If this code path adds
> refspec "foo:bar", and - say - setup_push_current() has already added
> refspec "foo:foo" (or simply "foo"), then do we end up pushing into
> "foo" or "bar"?

I think you answered your own question below with the "return".

>> @@ -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;
>
> I guess this return ensures that branch.*.push overrides push.default,
> but might there be other sources of add_refspec() that would
> complicate things?

The default-push-refspecs is meant to be used only when there is no
other stronger clue given by the user (e.g. refspec on the command
line, e.g. "git push there master", pushing with configured refspecs
on remote.$name.push), so I think it is a bug if somebody calls this
function when there is other source.

Re: [PATCH 6/6] push: honor branch.*.push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> @@ -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;
> +       }

The most obvious question comes first: what result can I expect when
this interacts with remote.<name>.push?

Why did you design this feature like this?  Will the user _not_ want
refspec mapping except when pushing out the current branch with a
plain "git push"?

Also, you managed to throw out all safety out the window.  What
happens when the user does:

  # on branch master, derived from origin
  $ git push ram

And branch.master.push is set to next?  Will you let her shoot herself
in the foot like this?

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> An earlier round of this change by mistake broke the safety for
>> "simple" mode we have had since day 1 of that mode to make sure that
>> the branch in the repository we update is set to be the one we fetch
>> and integrate with, but it has been fixed.
>
> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?

An operation that has to expect failure due to safety was disabled
by the broken version.  The squashed end result reverts that change
to the test, to make sure we did not break the safety.

Re: [PATCH 0/6] Reroll of rr/triangular-push-fix

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I hope (perhaps foolishly) to persuade you nevertheless. I fear that
> if this series solidifies before I get there, we'll be stuck with this
> stupid erroring-out behavior forever.

I do not think it is "stupid" at all.

'simple' is supposed to be an easy and safe default to help new
people.  Your original patch to change its semantics for the central
workflow from the current 'make sure upstream is set and set to the
same name' to 'anything goes' is making the mode more dangerous than
the corresponding 'upstream'.  Such a mode may have its place, but
labelling such a mode with rough edges as 'simple' and forcing it on
new people _is_ stupid, IMHO.

In any case, the good news is, if you start strict, and if it turns
out to be stricter than necessary, it is easier to loosen it later,
because nobody would be relying on an operation to _fail_.

If you start too loose without safety, however, it is a lot harder
to tighten it later when it turns out that safety helps new people.

Since the beginning of this series, our working assumption for
triangular-simple' has been that it can just turn into a straight
'current'.  But given that 'simple' is supposed to be an easy and
safe default for new people, I suspect that it should be a bit more
strict.

For example, if you are on a random topic branch and say "git push",
always pushing it out may not be a very sensible thing to do, and it
might be safer if we restrict 'triangular-simple' to push out the
current branch to the branch of the same name, but only when such a
branch already exists at the remote end, or something.

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> @@ -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;
>> +       }
>
> The most obvious question comes first: what result can I expect when
> this interacts with remote.<name>.push?

Now you bring it up, the branch.*.push may want to be more specific
(when I am on _this_ branch, do this) than remote.*.push (when I am
pushing to that remote, I want this to happen in general), but this
default codepath would not be exercised when you have remote.*.push,
so the logic may need to be moved higher up in the foodchain.

> Also, you managed to throw out all safety out the window.  What
> happens when the user does:
>
>   # on branch master, derived from origin
>   $ git push ram
>
> And branch.master.push is set to next?  Will you let her shoot herself
> in the foot like this?

It is not shooting in the foot, if branch.master.push is explicitly
set to update next.  I do not see any issue in that part.

But the relative strength betweenh branch.*.push and remote.*.push
may need to be thought out.  I haven't.

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

[Johan Herland]

On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>>> +       git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
>>
>> Isn't  ${3:-repo1} a bashism?
>
> I do not think so.  But now I looked at it again, I think I would
> use ${3-repo1} form in this case myself.  No caller passes an empty
> string to the third place.

Ok, I have to admit that I'm not at all sure where the line between sh
and bash goes when it comes to ${magic}... Is there any good
documentation on what is in sh and what is not?

...Johan



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

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

[Eric Sunshine]

On Mon, Jun 24, 2013 at 4:33 AM, Johan Herland <johan@herland.net> wrote:
> On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johan Herland <johan@herland.net> writes:
>>
>>>> +       git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
>>>
>>> Isn't  ${3:-repo1} a bashism?
>>
>> I do not think so.  But now I looked at it again, I think I would
>> use ${3-repo1} form in this case myself.  No caller passes an empty
>> string to the third place.
>
> Ok, I have to admit that I'm not at all sure where the line between sh
> and bash goes when it comes to ${magic}... Is there any good
> documentation on what is in sh and what is not?

POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

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

[Johan Herland]

On Mon, Jun 24, 2013 at 9:46 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Johan Herland wrote:
>>> +static void setup_push_current(struct remote *remote, struct branch *branch)
>>> +{
>>> +       if (!branch)
>>> +               die(_(message_detached_head_die), remote->name);
>>> +       add_refspec(branch->name);
>>
>> Here (and above) we add a refspec to tell Git exactly what to push
>> from the local end, and into what on the remote end.
>
> Nope, we add the refspec "foo", without the :destination part.  The
> remote end is unspecified (and defaults to "foo", but that is in the
> transport layer).

Ok, so "foo" is not always semantically equivalent to "foo:foo", and
when adding "foo:bar" it is always considered more specific than (and
superior to) "foo". I think that makes sense.

>> Is it possible to
>> end up with multiple simultaneous refspecs matching the same local
>> ref, but mapping to different remote refs? If so, which will win, and
>> does that make sense?
>
> It is impossible.  We either:
>
> - Get an explicit refspec from the user and never run
> setup_default_push_refspecs() to begin with.
>
> - Run setup_push_refspecs() and add *one* refspec depending on the
> push.default value.

Thanks, that's what I wanted to hear. But then, does it make sense to
say that we will only ever have exactly _one_ push refspec in the
current context, and we should therefore replace the "static const
char **refspec;" string array with a single "static const char
*refspec;" string? That would make it obvious that there is no room
for ambiguity with overlapping refspecs.

...Johan

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

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

[Johan Herland]

On Mon, Jun 24, 2013 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>>> An earlier round of this change by mistake broke the safety for
>>> "simple" mode we have had since day 1 of that mode to make sure that
>>> the branch in the repository we update is set to be the one we fetch
>>> and integrate with, but it has been fixed.
>>
>> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?
>
> An operation that has to expect failure due to safety was disabled
> by the broken version.  The squashed end result reverts that change
> to the test, to make sure we did not break the safety.

Ok, thanks.

...Johan

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

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

[Johan Herland]

On Mon, Jun 24, 2013 at 10:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jun 24, 2013 at 4:33 AM, Johan Herland <johan@herland.net> wrote:
>> On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Johan Herland <johan@herland.net> writes:
>>>
>>>>> +       git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
>>>>
>>>> Isn't  ${3:-repo1} a bashism?
>>>
>>> I do not think so.  But now I looked at it again, I think I would
>>> use ${3-repo1} form in this case myself.  No caller passes an empty
>>> string to the third place.
>>
>> Ok, I have to admit that I'm not at all sure where the line between sh
>> and bash goes when it comes to ${magic}... Is there any good
>> documentation on what is in sh and what is not?
>
> POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

Thanks! Learn something new every day, I guess. :)

...Johan

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

Re: [PATCH 0/6] Reroll of rr/triangular-push-fix

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> 'simple' is supposed to be an easy and safe default to help new
> people.  Your original patch to change its semantics for the central
> workflow from the current 'make sure upstream is set and set to the
> same name' to 'anything goes' is making the mode more dangerous than
> the corresponding 'upstream'.  Such a mode may have its place, but
> labelling such a mode with rough edges as 'simple' and forcing it on
> new people _is_ stupid, IMHO.

Oh, I agree that "anything goes" was the wrong approach.  However, I
think a sane default for branch.$branch.merge is a good way forward.

> In any case, the good news is, if you start strict, and if it turns
> out to be stricter than necessary, it is easier to loosen it later,
> because nobody would be relying on an operation to _fail_.

Okay,  I will quote you if you raise issues about "preserving how it
has historically been functioning" when I complete the upstream-fix
topic.

There's no need to stall this series then: [1/6] to [5/6] largely look
good-to-merge; drop [6/6], as it needs more thought.

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

[Ramkumar Ramachandra]

Johan Herland wrote:
> But then, does it make sense to
> say that we will only ever have exactly _one_ push refspec in the
> current context, and we should therefore replace the "static const
> char **refspec;" string array with a single "static const char
> *refspec;" string? That would make it obvious that there is no room
> for ambiguity with overlapping refspecs.

Multiple refspecs can be specified on the command-line; set_refspecs()
is responsible for calling add_refspec() multiple times for each
refspec, and _that_ is the primary use of the "refspec" variable.  The
single add_refspec() invocation in the push.default switch is a
special case that reuses the variable.

Re: [PATCH 6/6] push: honor branch.*.push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>>   # on branch master, derived from origin
>>   $ git push ram
>>
>> And branch.master.push is set to next?  Will you let her shoot herself
>> in the foot like this?
>
> It is not shooting in the foot, if branch.master.push is explicitly
> set to update next.  I do not see any issue in that part.

The question does not pertain to master being mapped to next; it
pertains to central-workflow versus triangular-workflow: origin versus
ram.  If the user has set push.default to upstream, she _expects_
triangular pushes to always be denied, and this is the first violation
of that rule.  I'm tilting towards building a dependency between
branch.<name>.push and push.default.

Re: [PATCH 6/6] push: honor branch.*.push

[Johan Herland]

On Mon, Jun 24, 2013 at 4:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Junio C Hamano wrote:
>>>   # on branch master, derived from origin
>>>   $ git push ram
>>>
>>> And branch.master.push is set to next?  Will you let her shoot herself
>>> in the foot like this?
>>
>> It is not shooting in the foot, if branch.master.push is explicitly
>> set to update next.  I do not see any issue in that part.
>
> The question does not pertain to master being mapped to next; it
> pertains to central-workflow versus triangular-workflow: origin versus
> ram.  If the user has set push.default to upstream, she _expects_
> triangular pushes to always be denied, and this is the first violation
> of that rule.  I'm tilting towards building a dependency between
> branch.<name>.push and push.default.

I haven't followed this topic closely, and the concern described below
is probably a consequence of that. Please ignore if my worries are
unfounded.

It seems to me like we're adding (or have already added) quite a bit
of config variables and command-line options for specifying (at
varying levels of specificity and overridability) either the remote to
push to ($remote), or what ref to push into on the remote
($remote_ref). It worries me that we allow $remote and $remote_ref to
be set _separately_ and at separate levels of specificity. I wonder if
this too easily allows users to shoot themselves in the foot by
specifying $remote in one place (e.g. on the command line), then (in
their mind - unrelated) specifying $remote_ref in another place (e.g.
branch.foo.push), and then being negatively surprised when the two
conspire to push into the "wrong" $remote and/or $remote_ref.

I haven't yet dug deep enough to figure out an obvious failure mode
(and I probably should not have sent this email until I'd found one),
but I wonder if we'd be better off forcing the $remote and $remote_ref
configuration for a given branch to appear as more of a single unit.

What if, when setting up tracking for a given branch, we immediately
specified its complete pull and push targets?

For example, when in a centralized workflow (e.g. push.default =
upstream) and we're checking out local branch foo from origin's foo,
we could set up the following configuration [1]:

[branch "foo"]
    pull = origin/foo
    push = origin/foo

In a triangular workflow (assuming we had configuration to specify
such, and also a default push remote), we could then instead set up
the following config:

[branch "foo"]
    pull = origin/foo
    push = my_public/foo

This leaves no ambiguity for even the most novice user as to the pull
and push targets for a given branch, and it's also easy to change it,
either by editing the config file directly, or by using hypothetical
commands:

  git branch foo --pulls-from=origin/bar
  git branch foo --pushes-to=other_repo/foo

Any "git pull" without arguments will fail if branch.<current>.pull is
unset or invalid. Likewise any "git push" without arguments will fail
if branch.<current>.push is unset or invalid.

Obviously, specifying the remote and/or refspec on the command-line
would still override, as it does today, but for the argument-less
forms of "git pull" and "git push", the hierarchy of options and
defaults being consulted to figure out $remote and $remote_ref would
be small and easily understandable.


...Johan


[1]: I do realize that I'm abusing the "foo/bar" notation to mean
"$remote/$ref", and that this does not work in the general case where
both remote names and ref names may contain slashes, or when remote
names don't correspond to eponymous ref namespaces...

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

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>>   # on branch master, derived from origin
>>>   $ git push ram
>>>
>>> And branch.master.push is set to next?  Will you let her shoot herself
>>> in the foot like this?
>>
>> It is not shooting in the foot, if branch.master.push is explicitly
>> set to update next.  I do not see any issue in that part.
>
> The question does not pertain to master being mapped to next; it
> pertains to central-workflow versus triangular-workflow: origin versus
> ram.  If the user has set push.default to upstream, she _expects_
> triangular pushes to always be denied,...

If the user said "git push" without an explicit request to push to
"ram", and if branch.master.pushremote was not set to "ram", and
still the command "git push" pushed the branch to "ram", then I
would understand what you are worried about, but otherwise I do not
see how what you are saying makes sense.

A safety valve is different from a straight-jacket.  If you are
working largely with a central repository and have push.default set
to upstream, are you now disallowed to push out things to other
places to ask help from your colleague to check your wip?  Why?

Or are you saying that with push.default set to upstream, only these
two forms should be allowed?

    $ git push ;# no destination, no refspec
    $ git push there ref:spec ;# both explicitly specified

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I haven't yet dug deep enough to figure out an obvious failure mode
> (and I probably should not have sent this email until I'd found one),
> but I wonder if we'd be better off forcing the $remote and $remote_ref
> configuration for a given branch to appear as more of a single unit.

That sounds sensible.  I could see perhaps we would want to require,
for branch.*.push to be effective, branch.*.pushremote must be set
(honestly speaking, branch.*.push is not my itch and I'd probably be
happier if we didn't add it in the first place, though ;-).

> What if, when setting up tracking for a given branch, we immediately
> specified its complete pull and push targets?
>
> For example, when in a centralized workflow (e.g. push.default =
> upstream) and we're checking out local branch foo from origin's foo,
> we could set up the following configuration [1]:
>
> [branch "foo"]
>     pull = origin/foo
>     push = origin/foo

They should both be refs/heads/foo, as these are meant to name the
name in _their_ repository.  I see what you are saying, but the
behaviour you want happens without branch.foo.push, and the addition
may be redundant.  I do not immediately see what it is buying us.

Other than that when the user stops being centralized and starts to
push to his own publishing repository, branch.*.push needs to be
removed in addition to flipping push.default from upstream to
current, that is.

> In a triangular workflow (assuming we had configuration to specify
> such, and also a default push remote), we could then instead set up
> the following config:
>
> [branch "foo"]
>     pull = origin/foo
>     push = my_public/foo

Again, these are both refs/heads/foo.

> This leaves no ambiguity for even the most novice user as to the pull
> and push targets for a given branch, and it's also easy to change it,
> either by editing the config file directly, or by using hypothetical
> commands:
>
>   git branch foo --pulls-from=origin/bar
>   git branch foo --pushes-to=other_repo/foo

But you need to do that for _all_ branches when you acquire your own
publishing point; isn't that a rather cumbersome usability glitch?

> Obviously, specifying the remote and/or refspec on the command-line
> would still override, as it does today, but for the argument-less
> forms of "git pull" and "git push", the hierarchy of options and
> defaults being consulted to figure out $remote and $remote_ref would
> be small and easily understandable.

Not really.

In addition to "you need to run around and change configuration for
all branches" issue, you can never do push.default=matching, if you
always set branch.foo.push and made it stronger than push.default,
no?

Re: [PATCH 6/6] push: honor branch.*.push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If the user said "git push" without an explicit request to push to
> "ram", and if branch.master.pushremote was not set to "ram", and
> still the command "git push" pushed the branch to "ram", then I
> would understand what you are worried about, but otherwise I do not
> see how what you are saying makes sense.

We currently have no system to differentiate between those two cases.

> A safety valve is different from a straight-jacket.  If you are
> working largely with a central repository and have push.default set
> to upstream, are you now disallowed to push out things to other
> places to ask help from your colleague to check your wip?  Why?

I've been harping about how this safety valve is poorly done, and I'm
the first person interested in loosening it.  But I do not think this
is the way to do it: drop in branch.<name>.push magic, and make
upstream suddenly work with triangular workflows?

> Or are you saying that with push.default set to upstream, only these
> two forms should be allowed?
>
>     $ git push ;# no destination, no refspec
>     $ git push there ref:spec ;# both explicitly specified

No, no.  What I meant is:

  From the documentation of push.default, I _expect_ upstream to kick
  in only in the first case.  In the second case, I _know_ that my
  push.default is inconsequential.

With branch.<name>.push magic, you've sneaked in a push refspec under
the carpet, and the first form suddenly doesn't care about
push.default anymore.  remote.<name>.push is also an under-the-carpet
implementation that I don't like: it's done in push_with_options()
which can completely bypass setup_default_push_refspecs(), shooting
the user in the face.

I want properly defined precedence and well-defined overall behavior.
Not sneaky stuff.  I don't have an answer, but will start a discussion
with some code soon.  In the meantime, since we've already figured out
this series, merge it without 6/6.

Thanks.

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If the user said "git push" without an explicit request to push to
>> "ram", and if branch.master.pushremote was not set to "ram", and
>> still the command "git push" pushed the branch to "ram", then I
>> would understand what you are worried about, but otherwise I do not
>> see how what you are saying makes sense.
>
> We currently have no system to differentiate between those two cases.

I am not sure what two cases you are talking about?

If you do not set anywhere (like branch.master.pushremote or
remote.pushdefault) to push to "ram", and if you did not say "git
push ram" but just said "git push", we will not push to "ram"
(otherwise it is broken).  So if the push is going to "ram", the
user must have asked us to push there, either via the command line,
or these explicit configuration variables.  And why do you need to
differenciate between the command line "ram" and configured "ram"?
After all, isn't the configuration merely a typesaver?  If you know
often push to "ram", you configure to push there.  If you don't, you
don't.

>> Or are you saying that with push.default set to upstream, only these
>> two forms should be allowed?
>>
>>     $ git push ;# no destination, no refspec
>>     $ git push there ref:spec ;# both explicitly specified
>
> No, no.  What I meant is:
>
>   From the documentation of push.default, I _expect_ upstream to kick
>   in only in the first case.  In the second case, I _know_ that my
>   push.default is inconsequential.

The push.default is meant to be in effect only when there is no
other stronger clue from the user for what to update (e.g. command
line refspec, remote.*.push).  Because branch.*.push weatherbaloon
patch did not have any documentation update, it did not say it is a
yet another way to explicitly configure the push destination branch.
Perhaps that led to your expectation for upstream to kick in.

Of course, that requires, as you earlier pointed out, that the logic
to read from branch.*.push need to be moved out of the push.default
logic (in the weatherbaloon patch) and hosted to do_push() where it
checks if there is remote->pushrefspec[].  If branch.*.push wants to
defeat remote.*.push, then it should be checked before deciding to
use that configured remote.*.push.

> I want properly defined precedence and well-defined overall behavior.

Of course.  There is no sneakiness.

As I already said, I personally do not know what branch.*.push buys
us, and will happy to drop 6/6.  It is merely a weatherbaloon patch.

Re: [PATCH 6/6] push: honor branch.*.push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If you do not set anywhere (like branch.master.pushremote or
> remote.pushdefault) to push to "ram", and if you did not say "git
> push ram" but just said "git push", we will not push to "ram"
> (otherwise it is broken).  So if the push is going to "ram", the
> user must have asked us to push there, either via the command line,
> or these explicit configuration variables.  And why do you need to
> differenciate between the command line "ram" and configured "ram"?
> After all, isn't the configuration merely a typesaver?  If you know
> often push to "ram", you configure to push there.  If you don't, you
> don't.

Yes, you're talking about configuration variables in general.  If the user says

  $ git status

expecting long-form output, but gets short-form output, the world
hasn't ended.  But if the user does

  $ git push -f

expecting the push to go to one place, when it actually goes to
another place, she would have killed a few co-workers.

I'm not saying that we need to differentiate between configuration
variables and CLI options; what I _am_ saying is that we need to think
twice about moving a CLI option to a configuration variable, precisely
because we do not differentiate between the two cases.

As I have said earlier, my philosophy to prevent disasters does not
involve erroring-out or denying for the most part; rather it focuses
on giving the user immediate feedback like:

  $ git push -f
  # pushing master:next to ram (press ^C to abort)

Another form of feedback is the refspec @{p[ush]}, which I'm
struggling to complete.

> The push.default is meant to be in effect only when there is no
> other stronger clue from the user for what to update (e.g. command
> line refspec, remote.*.push).

Yes, I know what it does currently.  With your patch, the list becomes
"push.default will be in effect only when there is no CLI refspec,
remote.<name>.push or branch.<name>.push in effect".  I'm wondering if
there's a more elegant way to do it.

> Of course, that requires, as you earlier pointed out, that the logic
> to read from branch.*.push need to be moved out of the push.default
> logic (in the weatherbaloon patch) and hosted to do_push() where it
> checks if there is remote->pushrefspec[].  If branch.*.push wants to
> defeat remote.*.push, then it should be checked before deciding to
> use that configured remote.*.push.

I suspect there's a better way to do this.

> As I already said, I personally do not know what branch.*.push buys
> us, and will happy to drop 6/6.  It is merely a weatherbaloon patch.

I hope we come out with a better version soon.

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

[Junio C Hamano]

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

> Johan Herland <johan@herland.net> writes:
>
>>> +       git --git-dir="${3:-repo1}" log -1 --format='%h %s' "$2" >actual &&
>>
>> Isn't  ${3:-repo1} a bashism?
>
> I do not think so.  But now I looked at it again, I think I would
> use ${3-repo1} form in this case myself.  No caller passes an empty
> string to the third place.

Actually, because the caller blindly does this:

    # $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" "$3"
    }

where it should be doing something like this:

            check_pushed_commit HEAD "$2" ${3+"$3"}

if it wants $3 to be "optional", ${3:-repo1} is needed here to work
it around.

So I'll leave it as-is for now.

Re: [PATCH 6/6] push: honor branch.*.push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I'm not saying that we need to differentiate between configuration
> variables and CLI options; what I _am_ saying is that we need to think
> twice about moving a CLI option to a configuration variable, precisely
> because we do not differentiate between the two cases.

With remote.pushdefault, I think the ship has long sailed.

Re: [PATCH 6/6] push: honor branch.*.push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> With remote.pushdefault, I think the ship has long sailed.

What's wrong with the "early feedback" approach I suggested?

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]

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:

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

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]

"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

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

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

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

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

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

[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