[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

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

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

[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

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

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

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

[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

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

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

[Ramkumar Ramachandra]

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

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

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

Thanks.

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

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

-- 
1.8.3.1.454.g30263f3.dirty

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

[Ramkumar Ramachandra]

The line

  test_config push.default upstream

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

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

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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The line
>
>   test_config push.default upstream
>
> appears unnecessarily in two tests, as the final test_push_failure sets
> push.default before pushing anyway.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5528-push-default.sh | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index 4736da8..69ce6bf 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -48,7 +48,6 @@ test_expect_success '"upstream" pushes to configured upstream' '
>  test_expect_success '"upstream" does not push on unconfigured remote' '
>  	git checkout master &&
>  	test_unconfig branch.master.remote &&
> -	test_config push.default upstream &&
>  	test_commit three &&
>  	test_push_failure upstream
>  '
> @@ -57,7 +56,6 @@ test_expect_success '"upstream" does not push on unconfigured branch' '
>  	git checkout master &&
>  	test_config branch.master.remote parent1 &&
>  	test_unconfig branch.master.merge &&
> -	test_config push.default upstream
>  	test_commit four &&
>  	test_push_failure upstream
>  '

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

    test_expect_success '"upstream" does not push when remotes do not match' '
            git checkout master &&
            test_config branch.master.remote parent1 &&
            test_config branch.master.merge refs/heads/foo &&
            test_config push.default upstream &&
            test_commit five &&
            test_must_fail git push parent2
    '

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

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

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