[RFC/PATCH] push: introduce implicit push

[RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Currently, there is no way to invoke 'git push' without explicitly
specifying the destination to push to as the first argument.  When
pushing several branches, this information is often available in
branch.<name>.pushremote, falling back to branch.<name>.remote.  So,
we can use this information to create a more pleasant push experience.
You can now do:

    $ git push master next pu

If the branches master, next, and pu have different remotes, do_push()
will be executed three times on the three different remotes.

NOTE:

Pushing a non-branch ref still uses the current branch's
configuration, and this is wrong.  We need to find a way to fix
remote_get_1() without breaking existing features.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This is a very rough patch, meant to illustrate how to build our
 implicit push feature.  I think it's a really good idea, and will
 polish the patch after I get feedback.

 builtin/push.c | 56 ++++++++++++++++++++++++++++++++++--
 remote.c       | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h       |  9 ++++++
 3 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 909c34d..e8b667c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -26,6 +26,12 @@ static int refspec_nr;
 static int refspec_alloc;
 static int default_matching_used;
 
+static void reset_refspecs()
+{
+	refspec_nr = 0;
+	refspec_alloc = 0;
+}
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;
@@ -277,6 +283,11 @@ static void advise_ref_needs_force(void)
 	advise(_(message_advice_ref_needs_force));
 }
 
+static int is_possible_refspec(const char *name) {
+	/* TODO: check that name looks like a refspec */
+	return !is_configured_remote(name) && !strstr(name, "://");
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -319,10 +330,9 @@ static int push_with_options(struct transport *transport, int flags)
 	return 1;
 }
 
-static int do_push(const char *repo, int flags)
+static int do_push(const char *repo, struct remote *remote, int flags)
 {
 	int i, errs;
-	struct remote *remote = pushremote_get(repo);
 	const char **url;
 	int url_nr;
 
@@ -414,6 +424,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int tags = 0;
 	int rc;
 	const char *repo = NULL;	/* default repository */
+	struct remote *remote;
+
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),
 		OPT_STRING( 0 , "repo", &repo, N_("repository"), N_("repository")),
@@ -455,11 +467,49 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		add_refspec("refs/tags/*");
 
 	if (argc > 0) {
+		if (!strcmp(argv[0], "--") || is_possible_refspec(argv[0])) {
+			struct refspec_with_remote *refspec_remotes;
+			int i, pushtimes;
+
+			/* Attempt to fetch remotes for each refspec */
+			if (!strcmp(argv[0], "--"))
+				set_refspecs(argv + 1, argc - 1);
+			else
+				set_refspecs(argv, argc);
+			refspec_remotes = pushremote_get_for_refspec(refspec_nr,
+								refspec);
+			if (!refspec_remotes)
+				die(_("internal error: refspec_remotes() returned NULL"));
+
+			/* TODO: collect the refspecs for each
+			 * remote and batch the do_push() for
+			 * each remote
+			 */
+			for (i = 0, pushtimes = refspec_nr; i < pushtimes; i++) {
+				struct strbuf sb = STRBUF_INIT;
+
+				/* TODO: clean up this nonsense */
+				if (refspec_remotes[i].refspec->dst)
+					strbuf_addf(&sb, "%s:%s",
+						refspec_remotes[i].refspec->src,
+						refspec_remotes[i].refspec->dst);
+				else
+					strbuf_addf(&sb, "%s",
+						refspec_remotes[i].refspec->src);
+				reset_refspecs();
+				set_refspecs((const char **) &sb.buf, 1);
+				rc = do_push(NULL, refspec_remotes[i].remote, flags);
+				if (rc == -1)
+					usage_with_options(push_usage, options);
+			}
+			return rc;
+		}
 		repo = argv[0];
+		remote = pushremote_get(repo);
 		set_refspecs(argv + 1, argc - 1);
 	}
 
-	rc = do_push(repo, flags);
+	rc = do_push(repo, remote, flags);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/remote.c b/remote.c
index 68eb99b..b51ac9b 100644
--- a/remote.c
+++ b/remote.c
@@ -50,6 +50,7 @@ static int branches_nr;
 static struct branch *current_branch;
 static const char *default_remote_name;
 static const char *pushremote_name;
+static const char *pushremote_for_branch;
 static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
@@ -345,6 +346,36 @@ static void read_branches_file(struct remote *remote)
 	remote->fetch_tags = 1; /* always auto-follow */
 }
 
+static int branch_config(const char *key, const char *value, void *data)
+{
+	const char *name;
+	const char *subkey;
+	struct branch *branch;
+	struct branch *this_branch;
+
+	if (!data)
+		return -1;
+	this_branch = (struct branch *) data;
+
+	if (!prefixcmp(key, "branch.")) {
+		name = key + 7;
+		subkey = strrchr(name, '.');
+		if (!subkey)
+			return 0;
+		branch = make_branch(name, subkey - name);
+		if (!strcmp(subkey, ".remote")) {
+			if (branch == this_branch &&
+				git_config_string(&pushremote_for_branch, key, value))
+				return -1;
+		} else if (!strcmp(subkey, ".pushremote")) {
+			if (branch == this_branch &&
+				git_config_string(&pushremote_for_branch, key, value))
+				return -1;
+		}
+	}
+	return 0;
+}
+
 static int handle_config(const char *key, const char *value, void *cb)
 {
 	const char *name;
@@ -682,6 +713,12 @@ static int valid_remote_nick(const char *name)
 	return !strchr(name, '/'); /* no slash */
 }
 
+int is_configured_remote(const char *name)
+{
+	read_config();
+	return valid_remote(make_remote(name, 0));
+}
+
 static struct remote *remote_get_1(const char *name, const char *pushremote_name)
 {
 	struct remote *ret;
@@ -727,6 +764,59 @@ struct remote *pushremote_get(const char *name)
 	return remote_get_1(name, pushremote_name);
 }
 
+struct refspec_with_remote *pushremote_get_for_refspec(int refspec_nr,
+						const char **refspec)
+{
+	struct refspec_with_remote *ret;
+	struct refspec *refspecs;
+	unsigned char sha1[20];
+	int i;
+
+	read_config();
+	ret = xcalloc(sizeof(struct refspec_with_remote), refspec_nr);
+	refspecs = parse_push_refspec(refspec_nr, refspec);
+
+	for (i = 0; i < refspec_nr; i++) {
+		const char *src, *dst;
+		struct remote *remote;
+
+		src = resolve_ref_unsafe(refspecs[i].src, sha1, 0, NULL);
+		if (prefixcmp(src, "refs/"))
+			src = resolve_ref_unsafe(mkpath("refs/heads/%s", src),
+						sha1, 1, NULL);
+		if (refspecs[i].dst) {
+			dst = resolve_ref_unsafe(refspecs[i].dst, sha1, 1, NULL);
+			if (prefixcmp(dst, "refs/"))
+				dst = resolve_ref_unsafe(mkpath("refs/heads/%s", dst),
+							sha1, 1, NULL);
+		}
+		if (!src || (refspecs[i].dst && !dst)) {
+			remote = pushremote_get(NULL);
+		} else {
+			struct branch *branch;
+
+			branch = make_branch(src + strlen("refs/heads/"), 0);
+			git_config(branch_config, branch);
+			if (!pushremote_for_branch)
+				die(_("Branch %s does not have a valid configured "
+						"pushremote or remote"), branch->name);
+
+			/* Prepare the remote */
+			remote = make_remote(pushremote_for_branch, 0);
+			if (!valid_remote(remote))
+				die(_("Branch %s has an invalid remote configured: %s"),
+					branch->name, remote->name);
+			remote->fetch = parse_fetch_refspec(remote->fetch_refspec_nr,
+							remote->fetch_refspec);
+			remote->push = parse_push_refspec(remote->push_refspec_nr,
+							remote->push_refspec);
+		}
+		ret[i].refspec = &refspecs[i];
+		ret[i].remote = remote;
+	}
+	return ret;
+}
+
 int remote_is_configured(const char *name)
 {
 	int i;
diff --git a/remote.h b/remote.h
index cf56724..e39cf1b 100644
--- a/remote.h
+++ b/remote.h
@@ -52,12 +52,15 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 struct remote *pushremote_get(const char *name);
+struct refspec_with_remote *pushremote_get_for_refspec(int nr_refspec,
+						const char **refspec);
 int remote_is_configured(const char *name);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
 
 int remote_has_url(struct remote *remote, const char *url);
+int is_configured_remote(const char *name);
 
 struct refspec {
 	unsigned force : 1;
@@ -69,6 +72,12 @@ struct refspec {
 	char *dst;
 };
 
+struct refspec_with_remote {
+	struct refspec *refspec;
+	struct remote *remote;
+	int collected:1;
+};
+
 extern const struct refspec *tag_refspec;
 
 struct ref *alloc_ref(const char *name);
-- 
1.8.2.1.340.g945b0ee.dirty

Re: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, there is no way to invoke 'git push' without explicitly
> specifying the destination to push to as the first argument.  When
> pushing several branches, this information is often available in
> branch.<name>.pushremote, falling back to branch.<name>.remote.  So,
> we can use this information to create a more pleasant push experience.
> You can now do:
>
>     $ git push master next pu
>
> If the branches master, next, and pu have different remotes, do_push()
> will be executed three times on the three different remotes.

I am lukewarm on this one, slightly more close to negative than
positive, for a couple of reasons.

The primary reason is the confusion factor Jeff mentioned in the
thread that inspired this patch.  People would realize it is very
natural to decide where to push to based on what branch is being
pushed, but only after they think it long and hard enough [*1*].  I
suspect that it is an equally natural expectation for casual users
that the destination is chosen based on the current branch, if only
because that is what they are used to seeing when they say "git
push" without any argument.

Even though I personally am in favor of this "destination is tied to
what is pushed out", not "destination is chosen based on the current
branch", I can understand why some people would prefer the latter,
and why they find it simpler and easier to explain.

The second reason is purely on the differences between what the
above clean-nice explanation says and what the patch actually does.

I think "is-possible-refspec" and "pushremote-get-for-refspec" are
both way over-engineered, even for people who agree with me and the
above introduction for this change to favor "destination depends on
what branch is pushed out".  If is-possible-refspec is replaced with
a much simpler to understand logic, "Is this a local branch name?",
possibly combined with "There is no such path on the filesystem" and
"It's not a defined remote" (iow, reject "git push master:next" and
anything more complex) [*2*], I suspect it would be a bit more
sellable.


[Footnote]

*1* I've explained in a separate message why basing the destination
on what is being pushed is logical and internally consistent.

*2* This is in the same spirit (not implementation or design) as
revname vs pathspec disambiguation.

Re: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The primary reason is the confusion factor Jeff mentioned in the
> thread that inspired this patch.  People would realize it is very
> natural to decide where to push to based on what branch is being
> pushed, but only after they think it long and hard enough [*1*].  I
> suspect that it is an equally natural expectation for casual users
> that the destination is chosen based on the current branch, if only
> because that is what they are used to seeing when they say "git
> push" without any argument.

I agree with you largely, but I would still argue that choosing a
destination based on the current branch is a historical mistake made
by "matching".  We don't have to be stuck with this historical
mistake, because this is a new syntax: when users read about it in the
documentation/ What's New in git.git type email, they will also learn
that it chooses the destination based on the refspec.

> Even though I personally am in favor of this "destination is tied to
> what is pushed out", not "destination is chosen based on the current
> branch", I can understand why some people would prefer the latter,
> and why they find it simpler and easier to explain.

Agreed.  This is a consequence of not introducing triangular workflows
earlier, and getting our users used to distributed workflows.  With
this patch, users must mandatorily know about remote.pushdefault and
branch.<name>.pushremote, if they want to work in multiple-remote
scenarios.  My argument for that is that multiple-remote workflows
have always been a hack until now, and users of that setup will thank
us for fixing this.

> The second reason is purely on the differences between what the
> above clean-nice explanation says and what the patch actually does.
>
> I think "is-possible-refspec" and "pushremote-get-for-refspec" are
> both way over-engineered, even for people who agree with me and the
> above introduction for this change to favor "destination depends on
> what branch is pushed out".  If is-possible-refspec is replaced with
> a much simpler to understand logic, "Is this a local branch name?",
> possibly combined with "There is no such path on the filesystem" and
> "It's not a defined remote" (iow, reject "git push master:next" and
> anything more complex) [*2*], I suspect it would be a bit more
> sellable.

I don't feel strongly either way, as I just want a simple 'git push
master next +pu' to DTRT.  I rarely, if ever, specify the :<dst> part
of the refpsec.  Just so we're clear, we want:

- In git push master, master is verified not to be a path on the
filesystem, not a remote, and finally a local branch.

- In git push master:next, master:next is interpreted as a destination.

- In git push master next:pu, master is verified as usual, and next:pu
is pushed to the remote specified by next.  My patch currently does
this (checks that <src> and <dst> are branches).

- In git push master next:refs/tags/v3.1 and git push master
v3.1:refs/heads/next, master is verified as usual and the refspec is
pushed to the remote specified by remote.pushdefault, falling back to
origin (since the <dst> is not a branch).  My patch currently pushes
it to the current branch's configuration, and I've already marked it
as a TODO.

Re: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I agree with you largely, but I would still argue that choosing a
> destination based on the current branch is a historical mistake made
> by "matching".

"matching" is not necessarily a good default for everybody, and we
are fixing that historical mistake in Git 2.0.

Step back and think again. "matching" has never been about where the
push goes.  It is about what are pushed out, once you decide where
to push.

If a user often wants to push out a branch as soon as he made a
commit on it (even when other branches that go to the same
publishing point should not be pushed out yet), an instructoin "Ok,
I'll push to that repository" that pushes all matching branches will
not work well, because it will publish uncooked other branches, too.

That is the historical mistake of making "matching" the unconfigured
default. The historical mistake of "matching" does not have anything
to do with the choice of "where to push". It is only about "what to
push".

If you re-read your three-remote example involving "upstream" (me),
"ram" (your wip publishing point) and "peff":

  Cf. http://thread.gmane.org/gmane.comp.version-control.git/220769/focus=220933

you'll see that the only reason why you thought you need the hack to
set pushremote to null was because you did _not_ use matching. It
illustrates one reason why blaming matching for selection of
destination misses the point.

In any case, dispelling a misplaced blame on "matching" is not the
main point of this message.

> With this patch, users must mandatorily know about
> remote.pushdefault and branch.<name>.pushremote, if they want to
> work in multiple-remote scenarios.

I am afraid that it is neither sufficient nor a good solution.

I do not necessarily think that the best course is to devise an
unintuitive (to unsuspecting users) set of rules and force users to
understand it. That is where my secondary unhappiness comes from,
and that was why I said that limiting the magic only to a very
simple and easy to understand case might make it more sellable.

"git push" that pays attention to "branch.*.remote" was the original
sin.

To casual users with push.default=current/upstream, the two branches
(my current branch?  the branch I am pushing out?) have always been
the same when branch.*.remote is used.  These users don't even have
to think about the distinction between the two [*1*].  But the
distinction starts mattering once you start wishing to omit saying
_where_ to push, because at that point, _what_ to push is the only
thing the user would give us, but the end users are not used to see
the destination chosen based on what is being pushed out.

The new branch.*.pushremote does not alleviate this confusion. It
gives the same "when on this branch, we push out to that remote"
(and not "when pushing this branch out, it goes there" impression.

The new remote.pushdefault _is_ a definite improvement: "If I do not
say where to push, this is where things go". It makes a very clear
statement, and does not have that confusion.

> - In git push master, master is verified not to be a path on the
> filesystem, not a remote, and finally a local branch.

Yes.

> - In git push master:next, master:next is interpreted as a destination.

Ideally it should notice and diagnose it as a syntax error and error
out (of course an attempt to locate master: URL handler will fail,
but that is less nice).

> - In git push master next:pu, master is verified as usual, and next:pu
> is pushed to the remote specified by next.  My patch currently does
> this (checks that <src> and <dst> are branches).

I am not sure what you mean by "and next:pu is...".  If "git push
next:pu master" should error out without mistaking "next:pu" as
destination, so should "git push master next:pu", I think.

The last one is also the same.  The "guess destination" magic should
kick in only when we can verify _all_ the refs we are pushing out
are simple ones (branch names, and possibly tag names), and the
behaviour should not depend on the order. Anything more complex is
too confusing.

I personally think it is much more sellable to use an even simpler
rule than what Jeff suggested, to make

	git push -- <refspec>

go to the remote.pushdefault (falling back to remote.default that is
"origin"), without even paying attention to what branch you are on,
and ignoring branch.*.remote/pushremote configuration.

That is sufficient to support the triangular, the publish-to-mine,
and the centralized workflows, no?  In any of these cases, the
repository you push out to is _one_, even though it may be a
different one from where you pull from.  If you have a very special
branch that goes to a different place than all the other branches,
you can always push out while on that branch without any refspec,
anyway.


[Footnote]

*1* If you really think about what branch.*.remote is _for_, it says
"I want this branch to integrate with that branch at this remote".
If the user were to push the branch out using the configuration, it
is more logical to think of it as "When pushing this branch out, it
goes there", and not as "When I am on this branch and say 'git
push', 'git push' pushes there".

But that is clear and logical _only_ to Git-heads who have thought
about this hard enough.

Re: [RFC/PATCH] push: introduce implicit push

[Jakub Narębski]

W dniu 14.04.2013 06:42, Junio C Hamano pisze:

> I personally think it is much more sellable to use an even simpler
> rule than what Jeff suggested, to make
> 
> 	git push -- <refspec>
> 
> go to the remote.pushdefault (falling back to remote.default that is
> "origin"), without even paying attention to what branch you are on,
> and ignoring branch.*.remote/pushremote configuration.
> 
> That is sufficient to support the triangular, the publish-to-mine,
> and the centralized workflows, no?  In any of these cases, the
> repository you push out to is _one_, even though it may be a
> different one from where you pull from.  If you have a very special
> branch that goes to a different place than all the other branches,
> you can always push out while on that branch without any refspec,
> anyway.

I think it also supports users of 'matching' that have push default
correctly configured.  Currently they can use "git push" in all cases
but first push of a branch, where they had to use "git push origin
branch", or "git push pushremote branch", and with those changes can
now simply use "git push -- branch".

Nice.

Here I think simpler is better, especially that diferent users
have different expectations: push to remote based on current branch or
push to remote or remotes based on branch or branches being pushed.
-- 
Jakub Narębski

Re: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]
> In any case, dispelling a misplaced blame on "matching" is not the
> main point of this message.

I _thought_ "matching" was a good scapegoat to blame current user
expectations on.  However, it's okay if you think that we're
misplacing the blame.  As long as we can agree that user expectations
are aligned with "choosing the remote based on the current branch",
we're good.  Let's not waste time attempting to dissect the reason for
this.

> I do not necessarily think that the best course is to devise an
> unintuitive (to unsuspecting users) set of rules and force users to
> understand it. That is where my secondary unhappiness comes from,
> and that was why I said that limiting the magic only to a very
> simple and easy to understand case might make it more sellable.

Note that I'm not married to the interface I'm proposing: we can
always have a git branch --set-destination-to, corresponding to git
branch --set-upstream-to.  I'm not sure what interface to come up for
tweaking remote.pushdefault though (since we have deemed that a
remote.default counterpart is unnecessary, we've never really thought
about the problem).

> The new branch.*.pushremote does not alleviate this confusion. It
> gives the same "when on this branch, we push out to that remote"
> (and not "when pushing this branch out, it goes there" impression.

branch.*.pushremote is a very new feature, and I doubt it's even
available to users yet (distribution package); therefore there is no
meaning set-in-stone that we cannot change quickly.  If you think
about it, it's completely illogical for branch.* options to depend on
the state of the worktree; I really think we should push for them to
be inherent branch properties: we can update the documentation
accordingly, if we agree on this.

I think the main point of disagreement is that I'm in favor of
respecting logic, while you're in favor of respecting user
expectations set by (what we acknowledge now as) historical mistakes.
 I'm not saying that we should _not_ respect user expectations, but
rather that we should find some way to mould our users' expectations
to align with the logical choice without causing unpleasantness.

> The last one is also the same.  The "guess destination" magic should
> kick in only when we can verify _all_ the refs we are pushing out
> are simple ones (branch names, and possibly tag names), and the
> behaviour should not depend on the order. Anything more complex is
> too confusing.

Okay, no problem.  Just so we're clear: the "guess destination magic"
will only kick in when all the refspecs specified are either tags or
branches, and are missing the :<dst> counterpart.  So, git push master
+implicit-push should work just fine.

> I personally think it is much more sellable to use an even simpler
> rule than what Jeff suggested, to make
>
>         git push -- <refspec>
>
> go to the remote.pushdefault (falling back to remote.default that is
> "origin"), without even paying attention to what branch you are on,
> and ignoring branch.*.remote/pushremote configuration.
>
> That is sufficient to support the triangular, the publish-to-mine,
> and the centralized workflows, no?  In any of these cases, the
> repository you push out to is _one_, even though it may be a
> different one from where you pull from.  If you have a very special
> branch that goes to a different place than all the other branches,
> you can always push out while on that branch without any refspec,
> anyway.

This is logically incorrect: you're essentially making branches and
tags equivalent, while this is clearly not the case.  Will I ever
create "throwaway tags" just on my local machine for ease of working,
that I desire not to publish anywhere?  Now, replace "tags" with
"branches" in that sentence, and you have a completely different
answer.  Will I ever want to send certain specific "tags" to a special
destination (for review, for instance)?  Again, "branches" gives you a
different answer.  Can I attach properties to tags?  Are there tag.*
variables that I can set?  Why, then, do branches have these
variables?  Because they're not tags!  The way we work with branches
is completely different from the way we work with tags.

Let me try to drive this point in even harder:

My local clone is never one repository, but a "composite repository"
containing object stores from multiple remotes mixed in.  The
fantastic thing about git is that I can use the same worktree/ index/
local refs to work with this composite, as if it were a single
repository.  However, I need a way to sort through this mixed object
store/ multiple remotes madness: and for this, I have different remote
refs, and local branch specific configuration variables.  Again, I
don't think of my repository as a whole, but rather as a collection of
related branches that each have a specific source and destination.  I
do _not_ have one global source, or one global destination: that's
just the two-remote case, and git allows me to have N remotes in the
general case.  What I meant by "triangular workflows" was not the
reduced case of a repository-wide triangle, but about many little
triangles associated with every branch.

What is the practical application for all this?  What if I always want
to send some specific branches to a different destination (say Gerrit
review, or my friend's "integration server")?  What if I never want to
publish some branches (say they contain sensitive information such as
private keys)?  What if my upstream gave me write access to some
specific branches (in which case, I don't want to push those branches
to my usual "fork destination")?  Why are you proposing to
artificially limit the implicit-push topic to not support these cases?
 They're not special or fringe cases, but the general case that git
was always designed for.

Moreover, in your attempt to design a compromise, you're inventing a
different precedence order!  Wait a minute: why should we compromise
in the first place?  We're not an old enterprise trying to satisfy a
myopic client specification for a living; developing git is our
full-time hobby (today is a Sunday, by the way), and we should not
build something that we're less-than-elated with.  In my proposal, the
precedence order branch.<name>.pushremote, remote.pushdefault,
branch.<name>.remote, remote.default, origin, remains the same: we
just want to change which branch that <name> refers to.  In my
opinion, it is a much more subtle change than the entirely new
precedence order that you're inventing.

Re: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> ...  In my proposal, the
> precedence order branch.<name>.pushremote, remote.pushdefault,
> branch.<name>.remote, remote.default, origin, remains the same: we
> just want to change which branch that <name> refers to.

That "changing the meaning of <name>" in the middle, and doing so
will be confusing to the users, is exactly the issue, isn't it?

> In my
> opinion, it is a much more subtle change than the entirely new
> precedence order that you're inventing.

Adding "--" has never been my itch. I just brought it up out of thin
air as a possible alternative that is less confusing.

If it does not work well, we do not have to add it, but it is
dubious that we would want to add something that is even more
confusing.

Just like Peff, I am sympathetic to people who want to omit "where
to" and have Git automatically make a guess, and would be happy if
we can find a reasonable solution to that problem.

But I am not convinced what we discussed in these threads are good
solutions. At least not yet.

Re: [RFC/PATCH] push: introduce implicit push

[Johannes Sixt]

Am 4/15/2013 5:04, schrieb Junio C Hamano:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
>> ...  In my proposal, the
>> precedence order branch.<name>.pushremote, remote.pushdefault,
>> branch.<name>.remote, remote.default, origin, remains the same: we
>> just want to change which branch that <name> refers to.
> 
> That "changing the meaning of <name>" in the middle, and doing so
> will be confusing to the users, is exactly the issue, isn't it?
> 
>> In my
>> opinion, it is a much more subtle change than the entirely new
>> precedence order that you're inventing.
> 
> Adding "--" has never been my itch. I just brought it up out of thin
> air as a possible alternative that is less confusing.

User says:

   git push -- master docs release

Then git pushes the three branches to three different upstreams. You find
that confusing. Do I understanding correctly so far?

If I were a push.default=(simple|upstream) type, then I would be totally
aware that there are three different upstreams involved because I had had
to configure them manually and explicitly (correct?), and I would be
completely surprised if the push would *not* go to three different upstreams.

Just my 2 cents. (But I'm a traditional "matching" type, so take this with
a grain of salt. Or I may be missing the point of this thread, as I
haven't followed closely.)

-- Hannes

Re: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> User says:
>
>    git push -- master docs release
>
> Then git pushes the three branches to three different upstreams. You find
> that confusing. Do I understanding correctly so far?

It is far more subtle than that.  User says that while on 'next'
branch.

The user has been trained to think "branch.master.remote" takes
effect while he has "master" branch checked out.

That is where the possible confusion comes from. As I said number of
times, you may be able to declare that the confusion is unfounded
once you think it through.

> Just my 2 cents. (But I'm a traditional "matching" type, so take this with
> a grain of salt. Or I may be missing the point of this thread, as I
> haven't followed closely.)

For exmaple, see

cf. http://thread.gmane.org/gmane.comp.version-control.git/218429/focus=220707

where I say "branch.next.remote" should not come into play when I
say "git push --master docs release" while on the next branch and
Peff gives a counter-viewpoint.

Re: [RFC/PATCH] push: introduce implicit push

[John Keeping]

On Mon, Apr 15, 2013 at 12:20:32AM -0700, Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > User says:
> >
> >    git push -- master docs release
> >
> > Then git pushes the three branches to three different upstreams. You find
> > that confusing. Do I understanding correctly so far?
> 
> It is far more subtle than that.  User says that while on 'next'
> branch.
> 
> The user has been trained to think "branch.master.remote" takes
> effect while he has "master" branch checked out.
> 
> That is where the possible confusion comes from. As I said number of
> times, you may be able to declare that the confusion is unfounded
> once you think it through.

I may be an atypical user, but my expectation currently is that
branch.<name>.remote is what is used when I run "git push" with no
additional arguments.

This is probably because whenever I add additional arguments (currently)
I have to specify where I am pushing to.

So I think breaking user expectations is a red herring here because the
current behaviour means that users cannot have any expectation of what
will happen in this case.  Either you don't say anything and "git push"
DTRT for your current branch or you must specify precisely what you want
to happen (or at least the remote to use if you have push.default =
matching or remote.<name>.mirror set).

Personally I'd vote for "git push -- master" pushing to
remote.pushdefault, but I really don't know how you handle "git push --"
with the naïve implementation of that - is it the same as "git push" or
"git push $(git config remote.pushdefault)"?

On the other hand, I'm really not sure how often I'd use this feature.
Normally I just use "git push" and if I'm giving any more arguments than
that then it's for something that I don't do often enough for it to be
worth setting up configuration.

Re: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

John Keeping wrote:
> I may be an atypical user, but my expectation currently is that
> branch.<name>.remote is what is used when I run "git push" with no
> additional arguments.
>
> This is probably because whenever I add additional arguments (currently)
> I have to specify where I am pushing to.
>
> So I think breaking user expectations is a red herring here because the
> current behaviour means that users cannot have any expectation of what
> will happen in this case.  Either you don't say anything and "git push"
> DTRT for your current branch or you must specify precisely what you want
> to happen (or at least the remote to use if you have push.default =
> matching or remote.<name>.mirror set).
>
> Personally I'd vote for "git push -- master" pushing to
> remote.pushdefault, but I really don't know how you handle "git push --"
> with the naïve implementation of that - is it the same as "git push" or
> "git push $(git config remote.pushdefault)"?

We're not changing, or even discussing, what a plain git push without
destination or refspecs specified should do (yes, that means git push
-- too); it depends on push.default, which already exists.  My
proposal does not aim to change the current behavior of _any_ current
invocation (that means git push, git push origin master, git push next
master v1.2, and so on). It aims to make the new syntax git push
master +next behave logically.  I think we can all agree that the
logical solution (leaving aside founded/ unfounded user expectations)
is to pick destinations for each of the branches specified
individually.  As I explained in my last email, using
remote.pushdefault is Wrong because it treats branches like tags, and
invents a new precedence.  Voting without a basis is useless: do you
have a counter-argument for the points I raised as to why it is Wrong?

Re: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> I may be an atypical user, but my expectation currently is that
> branch.<name>.remote is what is used when I run "git push" with no
> additional arguments.
>
> This is probably because whenever I add additional arguments (currently)
> I have to specify where I am pushing to.
>
> So I think breaking user expectations is a red herring here because the
> current behaviour means that users cannot have any expectation of what
> will happen in this case.

The thing is, people _want_ to reuse the knowledge they have already
learned to a situation it does not directly apply to, by finding a
consequence, natural extension of that knowledge, applied to a new
situation.

 - Your "branch.*.remote only kicks in when I do not say either what
   to push or where to push to, so 'git push -- master' won't be
   affected" could be one valid natural extension to your knowledge
   "the config only kicks in when I do not say either".

 - Peff's "'git push' chooses to push to branch.next.remote when I
   am on 'next', so 'git push -- master' run in the same state
   should also push to that place" is another equally valid natural
   extension to his knowledge that "'git push' chooses to push to
   branch.next.remote when I am on 'next'".

 - Ram's and my "branch.master.remote is about what remote my master
   branch integrates with, so no matter where I am, 'git push' that
   does not say where-to should push out my master to that remote"
   is yet another equally valid natural extension to our knowledge
   that ""branch.master.remote is about what remote my master branch
   integrates with".

I do not think it is a red-herring at all. It is not about
"breaking", but "there will be multiple, conflicting and equally
plausible expectations" that makes me worry about unnecessary
confusion.

Re: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> That "changing the meaning of <name>" in the middle, and doing so
> will be confusing to the users, is exactly the issue, isn't it?

Yes, but we have to change _something_ if we don't want to hit a WTF
like 'git push master next' pushing master and next to
branch.<HEAD>.pushremote.  In my opinion, this seems to be the less
evil (or disruptive) change.  After all, we're not proposing to change
the current behavior of any current git invocations: a plain git push
can still consider branch.<HEAD>.pushremote, and it's not a problem in
my opinion.  After all, a git fetch also considers
branch.<HEAD>.remote, and we all agree that this is fine.

1. We are changing the meaning of branch.<name>.remote, but this is
not inconsistent with the current behavior of push.default at all
(even push.default=matching).  We just have to improve the
push.default documentation.

2. We are not changing the meaning of _any_ existing git push
invocations.  Pushing "unrelated branches" to the "corresponding
remote" has not been possible until now (unless you check out each of
the branches, set push.default=current, and git push), and we're
inventing a new syntax that makes this possible.  I see no problem
with changing the meaning of branch.<name>.remote/pushremote for this
purpose.

> Just like Peff, I am sympathetic to people who want to omit "where
> to" and have Git automatically make a guess, and would be happy if
> we can find a reasonable solution to that problem.
>
> But I am not convinced what we discussed in these threads are good
> solutions. At least not yet.

There are only so many possibilities, Junio*.  You either decide that
the logical alternative that I proposed is too confusing and drop the
idea, or think about how to move forward minimizing friction.

* If you think there are other possibilities, I'd be glad to hear them.

Re: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I do not think it is a red-herring at all. It is not about
> "breaking", but "there will be multiple, conflicting and equally
> plausible expectations" that makes me worry about unnecessary
> confusion.

Well put.

My solution to the problem is to take one of the three alternatives,
and show how it plugs into the larger workflow picture; in this case,
how my proposal fits into the larger picture of a local clone being a
collection of branches, each having little triangles.

Re: [RFC/PATCH] push: introduce implicit push

[John Keeping]

On Mon, Apr 15, 2013 at 02:47:35PM +0530, Ramkumar Ramachandra wrote:
> John Keeping wrote:
> > I may be an atypical user, but my expectation currently is that
> > branch.<name>.remote is what is used when I run "git push" with no
> > additional arguments.
> >
> > This is probably because whenever I add additional arguments (currently)
> > I have to specify where I am pushing to.
> >
> > So I think breaking user expectations is a red herring here because the
> > current behaviour means that users cannot have any expectation of what
> > will happen in this case.  Either you don't say anything and "git push"
> > DTRT for your current branch or you must specify precisely what you want
> > to happen (or at least the remote to use if you have push.default =
> > matching or remote.<name>.mirror set).
> >
> > Personally I'd vote for "git push -- master" pushing to
> > remote.pushdefault, but I really don't know how you handle "git push --"
> > with the naïve implementation of that - is it the same as "git push" or
> > "git push $(git config remote.pushdefault)"?
> 
> We're not changing, or even discussing, what a plain git push without
> destination or refspecs specified should do (yes, that means git push
> -- too); it depends on push.default, which already exists.  My
> proposal does not aim to change the current behavior of _any_ current
> invocation (that means git push, git push origin master, git push next
> master v1.2, and so on). It aims to make the new syntax git push
> master +next behave logically.  I think we can all agree that the
> logical solution (leaving aside founded/ unfounded user expectations)
> is to pick destinations for each of the branches specified
> individually.  As I explained in my last email, using
> remote.pushdefault is Wrong because it treats branches like tags, and
> invents a new precedence.  Voting without a basis is useless: do you
> have a counter-argument for the points I raised as to why it is Wrong?

As Junio says in his parallel message, there are different opinions
here, my suggestions was to effectively replace "--" with the value of
remote.pushdefault.  I don't think your solution is not logical, but I
don't think it is the unique logical solution.

The problem is that people have different opinions of what the current
situation means, resulting in different expectations of what push
without a remote should do.  Whatever behaviour we choose /will/ be
surprising to some users, even though it is completely logical.  That
much is clear from the differing opinions in this thread.

Re: [RFC/PATCH] push: introduce implicit push

[John Keeping]

On Mon, Apr 15, 2013 at 02:29:29AM -0700, Junio C Hamano wrote:
>  - Your "branch.*.remote only kicks in when I do not say either what
>    to push or where to push to, so 'git push -- master' won't be
>    affected" could be one valid natural extension to your knowledge
>    "the config only kicks in when I do not say either".
> 
>  - Peff's "'git push' chooses to push to branch.next.remote when I
>    am on 'next', so 'git push -- master' run in the same state
>    should also push to that place" is another equally valid natural
>    extension to his knowledge that "'git push' chooses to push to
>    branch.next.remote when I am on 'next'".
> 
>  - Ram's and my "branch.master.remote is about what remote my master
>    branch integrates with, so no matter where I am, 'git push' that
>    does not say where-to should push out my master to that remote"
>    is yet another equally valid natural extension to our knowledge
>    that ""branch.master.remote is about what remote my master branch
>    integrates with".
> 
> I do not think it is a red-herring at all. It is not about
> "breaking", but "there will be multiple, conflicting and equally
> plausible expectations" that makes me worry about unnecessary
> confusion.

Okay, I think it's a red herring from my point of view that "this is
something new that is very different from the current functionality",
but as you point out, "no arguments vs. some arguments" is only one
potential internal model of the current behaviour.

So the question is "what is the natural extension of the current
behaviour?", and the answer for me is "it's completely new", but others
have different (and conflicting) internal models that give different
answers.

Re: [RFC/PATCH] push: introduce implicit push

[Felipe Contreras]

On Mon, Apr 15, 2013 at 4:59 AM, John Keeping <john@keeping.me.uk> wrote:

> So the question is "what is the natural extension of the current
> behaviour?", and the answer for me is "it's completely new", but others
> have different (and conflicting) internal models that give different
> answers.

I don't think this does anybody any service. If the current behavior
is wrong, and if users all over the Internet is any indication, it is;
we do not want to continue such bad behavior. If the new
functionality has a different behavior, it only makes sense to change
the old behavior to make it consistent.

Perpetuating bad behavior choices only hurt the users, we need to
start thinking how to make git more user friendly, not worry too much
about keep doing what we did five years ago.

Sure, we should not break user expectations from one day to the next,
but that doesn't mean we can't change anything ever.

-- 
Felipe Contreras

Re: [RFC/PATCH] push: introduce implicit push

[John Keeping]

On Mon, Apr 15, 2013 at 11:39:40AM -0500, Felipe Contreras wrote:
> On Mon, Apr 15, 2013 at 4:59 AM, John Keeping <john@keeping.me.uk> wrote:
> 
> > So the question is "what is the natural extension of the current
> > behaviour?", and the answer for me is "it's completely new", but others
> > have different (and conflicting) internal models that give different
> > answers.
> 
> I don't think this does anybody any service. If the current behavior
> is wrong, and if users all over the Internet is any indication, it is;
> we do not want to continue such bad behavior. If the new
> functionality has a different behavior, it only makes sense to change
> the old behavior to make it consistent.

The current "push.default = matching" behaviour may be wrong, but I
haven't seen anyone say that the fundamental "'git push' does something
depending on push.default" and "'git push there ref...' specifies
exactly what to do" is broken.

Re: [RFC/PATCH] push: introduce implicit push

[Felipe Contreras]

On Mon, Apr 15, 2013 at 12:13 PM, John Keeping <john@keeping.me.uk> wrote:
> On Mon, Apr 15, 2013 at 11:39:40AM -0500, Felipe Contreras wrote:
>> On Mon, Apr 15, 2013 at 4:59 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> > So the question is "what is the natural extension of the current
>> > behaviour?", and the answer for me is "it's completely new", but others
>> > have different (and conflicting) internal models that give different
>> > answers.
>>
>> I don't think this does anybody any service. If the current behavior
>> is wrong, and if users all over the Internet is any indication, it is;
>> we do not want to continue such bad behavior. If the new
>> functionality has a different behavior, it only makes sense to change
>> the old behavior to make it consistent.
>
> The current "push.default = matching" behaviour may be wrong, but I
> haven't seen anyone say that the fundamental "'git push' does something
> depending on push.default" and "'git push there ref...' specifies
> exactly what to do" is broken.

Maybe not, but that doesn't mean that the new behavior should be
limited by the old one. If there's an ideal new behavior, the old one
should eventually be updated to accommodate the new one.

-- 
Felipe Contreras

Re: [RFC/PATCH] push: introduce implicit push

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:

>> That "changing the meaning of <name>" in the middle, and doing so
>> will be confusing to the users, is exactly the issue, isn't it?
>
> Yes, but we have to change _something_ if we don't want to hit a WTF
> like 'git push master next' pushing master and next to
> branch.<HEAD>.pushremote.

Yep.  In the usual case, all relevant remotes are "origin" anyway, so
there's no confusion.  In the confusing cases it would be safer to
error out and give the user a hint about how to make the configuration
less confusing.

The manual could say:

	In olden times, each [branch "<name>"] section would often have
	its own remote and pushRemote settings.  Ordinary branch creation
	even created such a configuration.  Nowadays that is
	discouraged: we have found that it is less confusing in
	practice to:

	 A) Typically fetch from one remote "[remote] default = origin"
	    and push to another "[remote] pushDefault = personal".

	 B) In atypical cases, explicitly name the remote being used
	    on the command line.

Thinking more, I suspect there is an asymmetry between "fetch" and
"push" here that we missed.  The manual could say:

	In typical usage, a person ordinarily pushes to a single
	preferred publication point.  You can set your publication
	point using the "[remote] pushDefault" setting:

		[remote]
			pushDefault = myserver.example.com:/path/to/repo.git

	To push a collection of branches to that remote repository,
	pass a list of branch names to "git push" with a
	disambiguating "--" to ensure the first branch name is not
	treated as a remote name:

		git push -- master next pu

	For historical reasons, if "[remote] pushDefault" is not set,
	it defaults to the remote that the branch being pushed is set
	to pull from (its "upstream").  If pushDefault is unset and
	multiple branches being pushed have different upstream
	repositories, Git will error out to allow you to disambiguate.

	To push to a different remote repository, just name it
	explicitly on the command line.

		git push korg -- master next pu

The asymmetry is because a command like "git fetch -- master next pu"
doesn't make much sense, since you have to know what remote you
fetched from to act on the fetch result.

As you hinted before, this would involve reverting the introduction
of "branch.<name>.pushremote", with the explanation that it was a
mistake inspired by that false symmetry, that you noticed and were
uncomfortable with but the rest of us were blind too.

Does that make sense?

Jonathan

Re: [RFC/PATCH] push: introduce implicit push

[Jonathan Nieder]

Jonathan Nieder wrote:

> As you hinted before, this would involve reverting the introduction
> of "branch.<name>.pushremote", with the explanation that it was a
> mistake inspired by that false symmetry, that you noticed and were
> uncomfortable with but the rest of us were blind too.

s/too/to/

For the curious, here is the motivation for the weird syntax I
suggested:

	git push foo bar -- baz qux

Sure, I want "git push master" to work since it might reduce the
volume of questions on #git.  But selfishly, more important to me
was that it would simplify my life as I frequently do

	git push repo wagner vasks -- debian-experimental +candidate+patches

in three steps, and I'd rather do it in one.  I think of repo.or.cz,
wagner.debian.org, and vasks.debian.org as three different remotes and
like maintaining separate remote-tracking branches for them, so a
single remote/multiple urls setup didn't work as well when I tried it.

I should warn you that the sketch of a design I am replying to is
incomplete.  It doesn't say what should happen if you try

	git push -- master:theirmaster next:theirnext

Thanks,
Jonathan