threads / rfc / 33491

RFC patchpush: introduce implicit push

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

## tl;dr

21 messages between Apr 12, 2013 and Apr 16, 2013. Diffs are folded; open one to read it.

replies: 20people: 7as markdown or json

Ramkumar Ramachandra· Apr 12, 2013, 15:33 UTC · lore

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(-)
Show changes to 3 files +152 −3

builtin/push.c, remote.c, remote.h

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
Junio C Hamano· Apr 12, 2013, 22:28 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 11 quoted lines
> 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.

Ramkumar Ramachandra· Apr 13, 2013, 04:49 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 8 quoted lines
> 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.

Show 12 quoted lines
> 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.
Junio C Hamano· Apr 14, 2013, 04:42 UTC · re: Ramkumar Ramachandra · lore

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

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.

Jakub Narębski· Apr 14, 2013, 08:33 UTC · re: Junio C Hamano · lore

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

W dniu 14.04.2013 06:42, Junio C Hamano pisze:
Show 16 quoted lines
> 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
Ramkumar Ramachandra· Apr 14, 2013, 13:29 UTC · re: Junio C Hamano · lore

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

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.

Show 5 quoted lines
> 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.
Show 5 quoted lines
> 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.

Show 16 quoted lines
> 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.

Junio C Hamano· Apr 15, 2013, 03:04 UTC · re: Ramkumar Ramachandra · lore

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

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.

Johannes Sixt· Apr 15, 2013, 07:07 UTC · re: Junio C Hamano · lore

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

Am 4/15/2013 5:04, schrieb Junio C Hamano:
Show 16 quoted lines
> 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
Junio C Hamano· Apr 15, 2013, 07:20 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 6 quoted lines
> 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.

John Keeping· Apr 15, 2013, 08:35 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 15, 2013 at 12:20:32AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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.

Ramkumar Ramachandra· Apr 15, 2013, 09:17 UTC · re: John Keeping · lore

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

John Keeping wrote:
Show 18 quoted lines
> 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?

John Keeping· Apr 15, 2013, 09:46 UTC · re: Ramkumar Ramachandra · lore

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

On Mon, Apr 15, 2013 at 02:47:35PM +0530, Ramkumar Ramachandra wrote:
Show 33 quoted lines
> 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.

Junio C Hamano· Apr 15, 2013, 09:29 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 10 quoted lines
> 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.

Ramkumar Ramachandra· Apr 15, 2013, 09:44 UTC · re: Junio C Hamano · lore

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

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.

John Keeping· Apr 15, 2013, 09:59 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 15, 2013 at 02:29:29AM -0700, Junio C Hamano wrote:
Show 22 quoted lines
>  - 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.

Felipe Contreras· Apr 15, 2013, 16:39 UTC · re: John Keeping · lore

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

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
John Keeping· Apr 15, 2013, 17:13 UTC · re: Felipe Contreras · lore

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

On Mon, Apr 15, 2013 at 11:39:40AM -0500, Felipe Contreras wrote:
Show 12 quoted lines
> 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.

Felipe Contreras· Apr 15, 2013, 17:18 UTC · re: John Keeping · lore

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

On Mon, Apr 15, 2013 at 12:13 PM, John Keeping <john@keeping.me.uk> wrote:
Show 18 quoted lines
> 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
Ramkumar Ramachandra· Apr 15, 2013, 09:35 UTC · re: Junio C Hamano · lore

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

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.
Show 6 quoted lines
> 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.
Jonathan Nieder· Apr 16, 2013, 02:05 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
Show 6 quoted lines
>> 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
Jonathan Nieder· Apr 16, 2013, 02:13 UTC · re: Jonathan Nieder · lore

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

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

← back to recent threads