[PATCH 0/6] implement @{push} shorthand

[PATCH 0/6] implement @{push} shorthand

[Jeff King]

The basic idea is that in a triangular workflow, it's useful to be able
to refer to both @{upstream} and "the place I would push to" with a
shorthand. This idea was discussed over a year ago:

  http://thread.gmane.org/gmane.comp.version-control.git/240144/focus=240198

I've found it useful since then, so I decided to clean it up and submit
it.  This iteration renames it to "@{push}" (rather than "@{publish}"),
which is more descriptive. It's rebased, as there were some changes in
the area, and it also fixes some bugs in the original (of course, I may
also have _introduced_ bugs, as this is largely a rewrite that has not
had a year's worth of exercise).

I also took the opportunity to do some more aggressive refactoring of
the remote.c code, as there were some bits there that confused me every
time I looked at it. I think the result is easier to follow.

  [1/6]: remote.c: drop default_remote_name variable
  [2/6]: remote.c: drop "remote" pointer from "struct branch"
  [3/6]: remote.c: hoist branch.*.remote lookup out of remote_get_1
  [4/6]: remote.c: provide per-branch pushremote name
  [5/6]: sha1_name: refactor upstream_mark
  [6/6]: sha1_name: implement @{push} shorthand

The one thing I _didn't_ do here is refactor the logic in
builtin/push.c so what we could reuse it. There's still a switch
statement here on push_default. I don't like that, but I spent an hour
or so trying to pull out the push.c logic, and it got rather nasty.
There are a lot of global variables that get mutated by one-off
functions, and it all needs to happen in the right order. It looked like
any serious refactoring there is going to have a risk of regressions in
push.

So I punted on that. The simplified logic that is here in sha1_name is
not too bad, and I feel like we can unify the two at a later date if we
want to. And I'd much rather see bug reports like "@{push} doesn't act
like git-push" and deal with that, as opposed to "your series breaks
push".

-Peff

[PATCH 1/6] remote.c: drop default_remote_name variable

[Jeff King]

When we read the remote config from disk, we update a
default_remote_name variable if we see branch.*.remote
config for the current branch. This isn't wrong, or even all
that complicated, but it is a bit simpler (because it
reduces our overall state) to just lazily compute the
default when we need it.

The ulterior motive here is that the push config uses a
similar structure, and _is_ much more complicated as a
result. That will be simplified in a future patch, and it's
more readable if the logic remotes and push-remotes matches.

Note that we also used default_remote_name as a signal that
the remote config has been loaded; after this patch, we now
use an explicit flag.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index 68901b0..fcd868d 100644
--- a/remote.c
+++ b/remote.c
@@ -39,6 +39,8 @@ struct rewrites {
 	int rewrite_nr;
 };
 
+static int loaded_remotes_config;
+
 static struct remote **remotes;
 static int remotes_alloc;
 static int remotes_nr;
@@ -49,10 +51,8 @@ static int branches_alloc;
 static int branches_nr;
 
 static struct branch *current_branch;
-static const char *default_remote_name;
 static const char *branch_pushremote_name;
 static const char *pushremote_name;
-static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
 static struct rewrites rewrites_push;
@@ -367,12 +367,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			return 0;
 		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
-			if (git_config_string(&branch->remote_name, key, value))
-				return -1;
-			if (branch == current_branch) {
-				default_remote_name = branch->remote_name;
-				explicit_default_remote_name = 1;
-			}
+			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, ".pushremote")) {
 			if (branch == current_branch)
 				if (git_config_string(&branch_pushremote_name, key, value))
@@ -504,9 +499,11 @@ static void read_config(void)
 	unsigned char sha1[20];
 	const char *head_ref;
 	int flag;
-	if (default_remote_name) /* did this already */
+
+	if (loaded_remotes_config)
 		return;
-	default_remote_name = "origin";
+	loaded_remotes_config = 1;
+
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -708,8 +705,11 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name
 			name = pushremote_name;
 			name_given = 1;
 		} else {
-			name = default_remote_name;
-			name_given = explicit_default_remote_name;
+			if (current_branch && current_branch->remote_name) {
+				name = current_branch->remote_name;
+				name_given = 1;
+			} else
+				name = "origin";
 		}
 	}
 
-- 
2.4.0.rc0.363.gf9f328b

Re: [PATCH 1/6] remote.c: drop default_remote_name variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When we read the remote config from disk, we update a
> default_remote_name variable if we see branch.*.remote
> config for the current branch. This isn't wrong, or even all
> that complicated, but it is a bit simpler (because it
> reduces our overall state) to just lazily compute the
> default when we need it.
>
> The ulterior motive here is that the push config uses a
> similar structure, and _is_ much more complicated as a
> result. That will be simplified in a future patch, and it's
> more readable if the logic remotes and push-remotes matches.

I cannot quite parse the part after "if..."; "the logic used by
remotes and push-remotes matches"?

> Note that we also used default_remote_name as a signal that
> the remote config has been loaded; after this patch, we now
> use an explicit flag.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  remote.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 68901b0..fcd868d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -39,6 +39,8 @@ struct rewrites {
>  	int rewrite_nr;
>  };
>  
> +static int loaded_remotes_config;
> +

I expect that the reason why this is not a function scope static in
read_config() will be revealed in the later patch in this series...

Re: [PATCH 1/6] remote.c: drop default_remote_name variable

[Jeff King]

On Tue, Mar 31, 2015 at 01:37:35PM -0700, Junio C Hamano wrote:

> > When we read the remote config from disk, we update a
> > default_remote_name variable if we see branch.*.remote
> > config for the current branch. This isn't wrong, or even all
> > that complicated, but it is a bit simpler (because it
> > reduces our overall state) to just lazily compute the
> > default when we need it.
> >
> > The ulterior motive here is that the push config uses a
> > similar structure, and _is_ much more complicated as a
> > result. That will be simplified in a future patch, and it's
> > more readable if the logic remotes and push-remotes matches.
> 
> I cannot quite parse the part after "if..."; "the logic used by
> remotes and push-remotes matches"?

Sorry, it should have been "...if the logic for remotes and push-remotes
matches".

> > +static int loaded_remotes_config;
> > +
> 
> I expect that the reason why this is not a function scope static in
> read_config() will be revealed in the later patch in this series...

No, it's not used. I was leaving it as an "out" in case somebody ever
needed to reset it in order to re-read the config, but I agree it is
probably better to let them pull it out of the function if and when that
happens.

-Peff

[PATCH 2/6] remote.c: drop "remote" pointer from "struct branch"

[Jeff King]

When we create each branch struct, we fill in the
"remote_name" field from the config, and then fill in the
actual "remote" field based on that name. However, it turns
out that nobody really cares about this field. The only two
sites that access it are:

  1. git-merge, which uses it to notice when the branch does
     not have a remote defined. But we can easily replace this
     with looking at remote_name instead.

  2. remote.c itself, when setting up the @{upstream} merge
     config. But we don't need to save the "remote" for
     that; we can just look it up for the duration of the
     operation.

Getting rid of it drops one potential source of confusion:
is the value the match for "remote_name", or is it the
remote we would fetch from when on that branch (i.e., does
it fall back to "origin")?

When we add pushremote_name, this question would get even
more confusing, as pushremotes have a more complicated
lookup procedure. It would be nice for the code to be
consistent between the remote and pushremote, and this takes
us one step closer.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c |  2 +-
 remote.c        | 14 ++++++++------
 remote.h        |  1 -
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 3b0f8f9..1840317 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -955,7 +955,7 @@ static int setup_with_upstream(const char ***argv)
 
 	if (!branch)
 		die(_("No current branch."));
-	if (!branch->remote)
+	if (!branch->remote_name)
 		die(_("No remote for the current branch."));
 	if (!branch->merge_nr)
 		die(_("No default upstream defined for the current branch."));
diff --git a/remote.c b/remote.c
index fcd868d..d5fd605 100644
--- a/remote.c
+++ b/remote.c
@@ -1633,15 +1633,20 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 static void set_merge(struct branch *ret)
 {
+	struct remote *remote;
 	char *ref;
 	unsigned char sha1[20];
 	int i;
 
+	if (!ret->remote_name || !ret->merge_nr)
+		return;
+	remote = remote_get(ret->remote_name);
+
 	ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
 	for (i = 0; i < ret->merge_nr; i++) {
 		ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
 		ret->merge[i]->src = xstrdup(ret->merge_name[i]);
-		if (!remote_find_tracking(ret->remote, ret->merge[i]) ||
+		if (!remote_find_tracking(remote, ret->merge[i]) ||
 		    strcmp(ret->remote_name, "."))
 			continue;
 		if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
@@ -1661,11 +1666,8 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
-	if (ret && ret->remote_name) {
-		ret->remote = remote_get(ret->remote_name);
-		if (ret->merge_nr)
-			set_merge(ret);
-	}
+	if (ret)
+	       set_merge(ret);
 	return ret;
 }
 
diff --git a/remote.h b/remote.h
index 02d66ce..4bb6672 100644
--- a/remote.h
+++ b/remote.h
@@ -203,7 +203,6 @@ struct branch {
 	const char *refname;
 
 	const char *remote_name;
-	struct remote *remote;
 
 	const char **merge_name;
 	struct refspec **merge;
-- 
2.4.0.rc0.363.gf9f328b

Re: [PATCH 2/6] remote.c: drop "remote" pointer from "struct branch"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When we create each branch struct, we fill in the
> "remote_name" field from the config, and then fill in the
> actual "remote" field based on that name. However, it turns
> out that nobody really cares about this field. The only two
> sites that access it are:
>
>   1. git-merge, which uses it to notice when the branch does
>      not have a remote defined. But we can easily replace this
>      with looking at remote_name instead.
>
>   2. remote.c itself, when setting up the @{upstream} merge
>      config. But we don't need to save the "remote" for
>      that; we can just look it up for the duration of the
>      operation.
>
> Getting rid of it drops one potential source of confusion:
> is the value the match for "remote_name", or is it the
> remote we would fetch from when on that branch (i.e., does
> it fall back to "origin")?

I had to read the above three times before I realized that you were
wondering "what is the value of this 'remote' field?  is it what
remote_get() would give us for 'remote_name' and is NULL if
remote_name is not set, or is it never NULL and instead have the
remote for 'origin' if remote_name is not set?"

But perhaps it is just me.

We certainly have duplicated information between the two fields, and
it first looked somewhat unnatural that you kept the name with which
you need to trigger a search for the structure, instead of keeping
the structure, one of whose field is its name already.  Perhaps
there was a valid reason behind this choice, and I am guessing that
it is probably because it will not let you differenciate the case
where the user explicitly said 'origin' and we used 'origin' as a
fallback, if you keep a "remote" field that stores the instance of
the remote structure for 'origin' without keeping "remote_name".

But we shouldn't leave it for readers to guess.

> When we add pushremote_name, this question would get even
> more confusing, as pushremotes have a more complicated
> lookup procedure. It would be nice for the code to be
> consistent between the remote and pushremote, and this takes
> us one step closer.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/merge.c |  2 +-
>  remote.c        | 14 ++++++++------
>  remote.h        |  1 -
>  3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 3b0f8f9..1840317 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -955,7 +955,7 @@ static int setup_with_upstream(const char ***argv)
>  
>  	if (!branch)
>  		die(_("No current branch."));
> -	if (!branch->remote)
> +	if (!branch->remote_name)
>  		die(_("No remote for the current branch."));
>  	if (!branch->merge_nr)
>  		die(_("No default upstream defined for the current branch."));
> diff --git a/remote.c b/remote.c
> index fcd868d..d5fd605 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1633,15 +1633,20 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  
>  static void set_merge(struct branch *ret)
>  {
> +	struct remote *remote;
>  	char *ref;
>  	unsigned char sha1[20];
>  	int i;
>  
> +	if (!ret->remote_name || !ret->merge_nr)
> +		return;
> +	remote = remote_get(ret->remote_name);
> +
>  	ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
>  	for (i = 0; i < ret->merge_nr; i++) {
>  		ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
>  		ret->merge[i]->src = xstrdup(ret->merge_name[i]);
> -		if (!remote_find_tracking(ret->remote, ret->merge[i]) ||
> +		if (!remote_find_tracking(remote, ret->merge[i]) ||
>  		    strcmp(ret->remote_name, "."))
>  			continue;
>  		if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
> @@ -1661,11 +1666,8 @@ struct branch *branch_get(const char *name)
>  		ret = current_branch;
>  	else
>  		ret = make_branch(name, 0);
> -	if (ret && ret->remote_name) {
> -		ret->remote = remote_get(ret->remote_name);
> -		if (ret->merge_nr)
> -			set_merge(ret);
> -	}
> +	if (ret)
> +	       set_merge(ret);
>  	return ret;
>  }
>  
> diff --git a/remote.h b/remote.h
> index 02d66ce..4bb6672 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -203,7 +203,6 @@ struct branch {
>  	const char *refname;
>  
>  	const char *remote_name;
> -	struct remote *remote;
>  
>  	const char **merge_name;
>  	struct refspec **merge;

Re: [PATCH 2/6] remote.c: drop "remote" pointer from "struct branch"

[Jeff King]

On Tue, Mar 31, 2015 at 01:50:05PM -0700, Junio C Hamano wrote:

> > Getting rid of it drops one potential source of confusion:
> > is the value the match for "remote_name", or is it the
> > remote we would fetch from when on that branch (i.e., does
> > it fall back to "origin")?
> 
> I had to read the above three times before I realized that you were
> wondering "what is the value of this 'remote' field?  is it what
> remote_get() would give us for 'remote_name' and is NULL if
> remote_name is not set, or is it never NULL and instead have the
> remote for 'origin' if remote_name is not set?"
> 
> But perhaps it is just me.
> 
> We certainly have duplicated information between the two fields, and
> it first looked somewhat unnatural that you kept the name with which
> you need to trigger a search for the structure, instead of keeping
> the structure, one of whose field is its name already.  Perhaps
> there was a valid reason behind this choice, and I am guessing that
> it is probably because it will not let you differenciate the case
> where the user explicitly said 'origin' and we used 'origin' as a
> fallback, if you keep a "remote" field that stores the instance of
> the remote structure for 'origin' without keeping "remote_name".

That is the reason I was trying to explain above. Though I suppose you
could argue that remote_name suffers the same question (i.e., would we
ever set it to "origin"?)

It is much worse for pushremotes, which can come from
branch.*.pushremote, remote.pushdefault, branch.*.remote, or "origin".

I'll try to re-word the commit message.

-Peff

Re: [PATCH 2/6] remote.c: drop "remote" pointer from "struct branch"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 31, 2015 at 01:50:05PM -0700, Junio C Hamano wrote:
>
>> it first looked somewhat unnatural that you kept the name with which
>> you need to trigger a search for the structure, instead of keeping
>> the structure, one of whose field is its name already.
> ...
> That is the reason I was trying to explain above. Though I suppose you
> could argue that remote_name suffers the same question (i.e., would we
> ever set it to "origin"?)

Well, another would be that by keeping remote_name and making remote
on-demand, we may still have to keep all the defined branches in
core but we do not have to instanciate all the remotes, if each
branch only knows the remote_name.  A single look-up may be cheap
but that is not a good reason to do one-per-each-branch if we do not
need to.

> It is much worse for pushremotes, which can come from
> branch.*.pushremote, remote.pushdefault, branch.*.remote, or "origin".
>
> I'll try to re-word the commit message.

Thanks.

[PATCH 3/6] remote.c: hoist branch.*.remote lookup out of remote_get_1

[Jeff King]

We'll want to use this logic as a fallback when looking up
the pushremote, so let's pull it out into its own function.

We don't technically need to make this available outside of
remote.c, but doing so will provide a consistent API with
pushremote_for_branch, which we will add later.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 21 ++++++++++++++-------
 remote.h |  1 +
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index d5fd605..9555904 100644
--- a/remote.c
+++ b/remote.c
@@ -693,6 +693,18 @@ static int valid_remote_nick(const char *name)
 	return !strchr(name, '/'); /* no slash */
 }
 
+const char *remote_for_branch(struct branch *branch, int *explicit)
+{
+	if (branch && branch->remote_name) {
+		if (explicit)
+			*explicit = 1;
+		return branch->remote_name;
+	}
+	if (explicit)
+		*explicit = 0;
+	return "origin";
+}
+
 static struct remote *remote_get_1(const char *name, const char *pushremote_name)
 {
 	struct remote *ret;
@@ -704,13 +716,8 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name
 		if (pushremote_name) {
 			name = pushremote_name;
 			name_given = 1;
-		} else {
-			if (current_branch && current_branch->remote_name) {
-				name = current_branch->remote_name;
-				name_given = 1;
-			} else
-				name = "origin";
-		}
+		} else
+			name = remote_for_branch(current_branch, &name_given);
 	}
 
 	ret = make_remote(name, 0);
diff --git a/remote.h b/remote.h
index 4bb6672..2a7e7a6 100644
--- a/remote.h
+++ b/remote.h
@@ -211,6 +211,7 @@ struct branch {
 };
 
 struct branch *branch_get(const char *name);
+const char *remote_for_branch(struct branch *branch, int *explicit);
 
 int branch_has_merge_config(struct branch *branch);
 int branch_merge_matches(struct branch *, int n, const char *);
-- 
2.4.0.rc0.363.gf9f328b

[PATCH 4/6] remote.c: provide per-branch pushremote name

[Jeff King]

When remote.c loads its config, it records the
branch.*.pushremote for the current branch along with the
global remote.pushDefault value, and then binds them into a
single value: the default push for the current branch. We
then pass this value (which may be NULL) to remote_get_1
when looking up a remote for push.

This has a few downsides:

  1. It's confusing. The early-binding of the "current
     value" led to bugs like the one fixed by 98b406f
     (remote: handle pushremote config in any order,
     2014-02-24). And the fact that pushremotes fall back to
     ordinary remotes is not explicit at all; it happens
     because remote_get_1 cannot tell the difference between
     "we are not asking for the push remote" and "there is
     no push remote configured".

  2. It throws away intermediate data. After read_config()
     finishes, we have no idea what the value of
     remote.pushDefault was, because the string has been
     overwritten by the current branch's
     branch.*.pushremote.

  3. It doesn't record other data. We don't note the
     branch.*.pushremote value for anything but the current
     branch.

Let's make this more like the fetch-remote config. We'll
record the pushremote for each branch, and then explicitly
compute the correct remote for the current branch at the
time of reading.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 41 +++++++++++++++++++++++------------------
 remote.h |  2 ++
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/remote.c b/remote.c
index 9555904..8363bdb 100644
--- a/remote.c
+++ b/remote.c
@@ -51,7 +51,6 @@ static int branches_alloc;
 static int branches_nr;
 
 static struct branch *current_branch;
-static const char *branch_pushremote_name;
 static const char *pushremote_name;
 
 static struct rewrites rewrites;
@@ -369,9 +368,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (!strcmp(subkey, ".remote")) {
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, ".pushremote")) {
-			if (branch == current_branch)
-				if (git_config_string(&branch_pushremote_name, key, value))
-					return -1;
+			return git_config_string(&branch->pushremote_name, key, value);
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
@@ -511,10 +508,6 @@ static void read_config(void)
 		current_branch = make_branch(head_ref, 0);
 	}
 	git_config(handle_config, NULL);
-	if (branch_pushremote_name) {
-		free((char *)pushremote_name);
-		pushremote_name = branch_pushremote_name;
-	}
 	alias_all_urls();
 }
 
@@ -705,20 +698,32 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
 	return "origin";
 }
 
-static struct remote *remote_get_1(const char *name, const char *pushremote_name)
+const char *pushremote_for_branch(struct branch *branch, int *explicit)
+{
+	if (branch && branch->pushremote_name) {
+		if (explicit)
+			*explicit = 1;
+		return branch->pushremote_name;
+	}
+	if (pushremote_name) {
+		if (explicit)
+			*explicit = 1;
+		return pushremote_name;
+	}
+	return remote_for_branch(branch, explicit);
+}
+
+static struct remote *remote_get_1(const char *name, int for_push)
 {
 	struct remote *ret;
 	int name_given = 0;
 
 	if (name)
 		name_given = 1;
-	else {
-		if (pushremote_name) {
-			name = pushremote_name;
-			name_given = 1;
-		} else
-			name = remote_for_branch(current_branch, &name_given);
-	}
+	else
+		name = for_push ?
+		       pushremote_for_branch(current_branch, &name_given) :
+		       remote_for_branch(current_branch, &name_given);
 
 	ret = make_remote(name, 0);
 	if (valid_remote_nick(name)) {
@@ -739,13 +744,13 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name
 struct remote *remote_get(const char *name)
 {
 	read_config();
-	return remote_get_1(name, NULL);
+	return remote_get_1(name, 0);
 }
 
 struct remote *pushremote_get(const char *name)
 {
 	read_config();
-	return remote_get_1(name, pushremote_name);
+	return remote_get_1(name, 1);
 }
 
 int remote_is_configured(const char *name)
diff --git a/remote.h b/remote.h
index 2a7e7a6..30a11da 100644
--- a/remote.h
+++ b/remote.h
@@ -203,6 +203,7 @@ struct branch {
 	const char *refname;
 
 	const char *remote_name;
+	const char *pushremote_name;
 
 	const char **merge_name;
 	struct refspec **merge;
@@ -212,6 +213,7 @@ struct branch {
 
 struct branch *branch_get(const char *name);
 const char *remote_for_branch(struct branch *branch, int *explicit);
+const char *pushremote_for_branch(struct branch *branch, int *explicit);
 
 int branch_has_merge_config(struct branch *branch);
 int branch_merge_matches(struct branch *, int n, const char *);
-- 
2.4.0.rc0.363.gf9f328b

Re: [PATCH 4/6] remote.c: provide per-branch pushremote name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Let's make this more like the fetch-remote config. We'll
> record the pushremote for each branch, and then explicitly
> compute the correct remote for the current branch at the
> time of reading.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  remote.c | 41 +++++++++++++++++++++++------------------
>  remote.h |  2 ++
>  2 files changed, 25 insertions(+), 18 deletions(-)

Very nicely done.  Thanks.

[PATCH 5/6] sha1_name: refactor upstream_mark

[Jeff King]

We will be adding new mark types in the future, so separate
the suffix data from the logic.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_name.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6d10f05..3741ca3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -415,12 +415,12 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
-static inline int upstream_mark(const char *string, int len)
+static inline int at_mark(const char *string, int len,
+			  const char **suffix, int nr)
 {
-	const char *suffix[] = { "@{upstream}", "@{u}" };
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
+	for (i = 0; i < nr; i++) {
 		int suffix_len = strlen(suffix[i]);
 		if (suffix_len <= len
 		    && !memcmp(string, suffix[i], suffix_len))
@@ -429,6 +429,12 @@ static inline int upstream_mark(const char *string, int len)
 	return 0;
 }
 
+static inline int upstream_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{upstream}", "@{u}" };
+	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
 
-- 
2.4.0.rc0.363.gf9f328b

[PATCH 6/6] sha1_name: implement @{push} shorthand

[Jeff King]

In a triangular workflow, each branch may have two distinct
points of interest: the @{upstream} that you normally pull
from, and the destination that you normally push to. There
isn't a shorthand for the latter, but it's useful to have.

For instance, you may want to know which commits you haven't
pushed yet:

  git log @{push}..

Or as a more complicated example, imagine that you normally
pull changes from origin/master (which you set as your
@{upstream}), and push changes to your own personal fork
(e.g., as myfork/topic). You may push to your fork from
multiple machines, requiring you to integrate the changes
from the push destination, rather than upstream. With this
patch, you can just do:

  git rebase @{push}

rather than typing out the full name.

Signed-off-by: Jeff King <peff@peff.net>
---
As an aside, I messed up an &&-chain in the tests, which was caught by
GIT_TEST_CHAIN_LINT before submitting. The system works!

 Documentation/revisions.txt |  25 +++++++++++
 sha1_name.c                 | 102 +++++++++++++++++++++++++++++++++++++++++++-
 t/t1514-rev-parse-push.sh   |  63 +++++++++++++++++++++++++++
 3 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100755 t/t1514-rev-parse-push.sh

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 0796118..5d9df25 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.
   `branch.<name>.merge`).  A missing branchname defaults to the
   current one.
 
+'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
+  The suffix `@{push}` reports the branch "where we would push to" if
+  `git push` were run while `branchname` was checked out (or the current
+  `HEAD` is no branchname is specified). Since our push destination is
+  in a remote repository, of course, we report the local tracking branch
+  that corresponds to that branch (i.e., something in `refs/remotes/`).
++
+Here's an example to make it more clear:
++
+------------------------------
+$ git config push.default current
+$ git config remote.pushdefault myfork
+$ git checkout -b mybranch origin/master
+
+$ git rev-parse --symbolic-full-name @{upstream}
+refs/remotes/origin/master
+
+$ git rev-parse --symbolic-full-name @{push}
+refs/remotes/myfork/mybranch
+------------------------------
++
+Note in the example that we set up a triangular workflow, where we pull
+from one location and push to another. In a non-triangular workflow,
+`@{push}` is the same as `@{upstream}`, and there is no need for it.
+
 '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
   that commit object.  '{caret}<n>' means the <n>th parent (i.e.
diff --git a/sha1_name.c b/sha1_name.c
index 3741ca3..1da3291 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -435,6 +435,12 @@ static inline int upstream_mark(const char *string, int len)
 	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
+static inline int push_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{push}" };
+	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
 
@@ -482,7 +488,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 					nth_prior = 1;
 					continue;
 				}
-				if (!upstream_mark(str + at, len - at)) {
+				if (!upstream_mark(str + at, len - at) &&
+				    !push_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -1104,6 +1111,95 @@ static int interpret_upstream_mark(const char *name, int namelen,
 	return len + at;
 }
 
+static char *tracking_ref_for(struct remote *remote, const char *refname)
+{
+	char *ret;
+
+	ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
+	if (!ret)
+		die(_("@{push} has no local tracking branch for remote '%s'"),
+		    refname);
+	return ret;
+}
+
+static char *get_push_branch(const char *name, int len)
+{
+	char *branch_name;
+	struct branch *branch;
+	struct remote *remote;
+
+	branch_name = xmemdupz(name, len);
+	branch = branch_get(*branch_name ? branch_name : NULL);
+	if (!branch)
+		die(_("HEAD does not point to a branch"));
+	free(branch_name);
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		die(_("branch '%s' has no remote for pushing"), branch->name);
+
+	if (remote->push_refspec_nr) {
+		char *dst, *ret;
+
+		dst = apply_refspecs(remote->push, remote->push_refspec_nr,
+				     branch->refname);
+		if (!dst)
+			die(_("push refspecs for '%s' do not include '%s'"),
+			    remote->name, branch->name);
+
+		ret = tracking_ref_for(remote, dst);
+		free(dst);
+		return ret;
+	}
+
+	if (remote->mirror)
+		return tracking_ref_for(remote, branch->refname);
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		die(_("@{push} has no destination (push.default is 'nothing'"));
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return tracking_ref_for(remote, branch->refname);
+
+	case PUSH_DEFAULT_UPSTREAM:
+		return xstrdup(get_upstream_branch(name, len));
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up = get_upstream_branch(name, len);
+			char *cur = tracking_ref_for(remote, branch->refname);
+			if (strcmp(cur, up))
+				die("cannot resolve 'simple' @{push} to a single destination");
+			return cur;
+		}
+	}
+
+	die("BUG: unhandled @{push} situation");
+}
+
+static int interpret_push_mark(const char *name, int namelen,
+			       int at, struct strbuf *buf)
+{
+	int len;
+	char *result;
+
+	len = push_mark(name + at, namelen - at);
+	if (!len)
+		return -1;
+
+	if (memchr(name, ':', at))
+		return -1;
+
+	result = get_push_branch(name, at);
+	set_shortened_ref(buf, result);
+	free(result);
+
+	return len + at;
+}
+
 /*
  * This reads short-hand syntax that not only evaluates to a commit
  * object name, but also can act as if the end user spelled the name
@@ -1154,6 +1250,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 		len = interpret_upstream_mark(name, namelen, at - name, buf);
 		if (len > 0)
 			return len;
+
+		len = interpret_push_mark(name, namelen, at - name, buf);
+		if (len > 0)
+			return len;
 	}
 
 	return -1;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
new file mode 100755
index 0000000..7214f5b
--- /dev/null
+++ b/t/t1514-rev-parse-push.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='test <branch>@{push} syntax'
+. ./test-lib.sh
+
+resolve () {
+	echo "$2" >expect &&
+	git rev-parse --symbolic-full-name "$1" >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'setup' '
+	git init --bare parent.git &&
+	git init --bare other.git &&
+	git remote add origin parent.git &&
+	git remote add other other.git &&
+	test_commit base &&
+	git push origin HEAD &&
+	git branch --set-upstream-to=origin/master master &&
+	git branch --track topic origin/master &&
+	git push origin topic &&
+	git push other topic
+'
+
+test_expect_success '@{push} with default=nothing' '
+	test_config push.default nothing &&
+	test_must_fail git rev-parse master@{push}
+'
+
+test_expect_success '@{push} with default=simple' '
+	test_config push.default simple &&
+	resolve master@{push} refs/remotes/origin/master
+'
+
+test_expect_success 'triangular @{push} fails with default=simple' '
+	test_config push.default simple &&
+	test_must_fail git rev-parse topic@{push}
+'
+
+test_expect_success '@{push} with default=current' '
+	test_config push.default current &&
+	resolve topic@{push} refs/remotes/origin/topic
+'
+
+test_expect_success '@{push} with default=matching' '
+	test_config push.default matching &&
+	resolve topic@{push} refs/remotes/origin/topic
+'
+
+test_expect_success '@{push} with pushremote defined' '
+	test_config push.default current &&
+	test_config branch.topic.pushremote other &&
+	resolve topic@{push} refs/remotes/other/topic
+'
+
+test_expect_success '@{push} with push refspecs' '
+	test_config push.default nothing &&
+	test_config remote.origin.push refs/heads/*:refs/heads/magic/* &&
+	git push &&
+	resolve topic@{push} refs/remotes/origin/magic/topic
+'
+
+test_done
-- 
2.4.0.rc0.363.gf9f328b

Re: [PATCH 6/6] sha1_name: implement @{push} shorthand

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 0796118..5d9df25 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.
>    `branch.<name>.merge`).  A missing branchname defaults to the
>    current one.
>  
> +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> +  The suffix `@{push}` reports the branch "where we would push to" if

The corresponding description for upstream begins like this:

  The suffix '@\{upstream\}' to a branchname (short form '<branchname>@\{u\}')

and makes me wonder if the existing backslashes are unnecessary, or
if you forgot to use them in the new text.

> @@ -1104,6 +1111,95 @@ static int interpret_upstream_mark(const char *name, int namelen,
>  	return len + at;
>  }
>  
> +static char *tracking_ref_for(struct remote *remote, const char *refname)
> +{
> +	char *ret;
> +
> +	ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
> +	if (!ret)
> +		die(_("@{push} has no local tracking branch for remote '%s'"),
> +		    refname);

I would imagine that it would be very plausible that anybody with a
specific remote and the name of the ref that appears on that remote
would want to learn the local name of the remote-tracking ref we use
to track it.

But the error message limits the callers only to those who are
involved in @{push} codepath.  Shouldn't the error check be done in
the caller instead, anticipating the day this useful function ceases
to be static?

I would suspect that such a change would make it just a one-liner,
but I think this helper that takes remote and their refname is much
easier to read than four inlined calls to apply_refspecs() that have
to spell out remote->fetch, remote->fetch_refspec_nr separately.

Perhaps we would want 

	struct refspecs {
        	int nr, alloc;
                const char **refspec;
	} fetch_refspec;

in "struct remote", instead of these two separate fields, and then
make apply_refspecs() take "struct refspecs *"?  I haven't checked
and thought enough to decide if we want "struct refspec *" also in
that new struct, though.

Re: [PATCH 6/6] sha1_name: implement @{push} shorthand

[Jeff King]

On Tue, Mar 31, 2015 at 02:37:25PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> > index 0796118..5d9df25 100644
> > --- a/Documentation/revisions.txt
> > +++ b/Documentation/revisions.txt
> > @@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.
> >    `branch.<name>.merge`).  A missing branchname defaults to the
> >    current one.
> >  
> > +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> > +  The suffix `@{push}` reports the branch "where we would push to" if
> 
> The corresponding description for upstream begins like this:
> 
>   The suffix '@\{upstream\}' to a branchname (short form '<branchname>@\{u\}')
> 
> and makes me wonder if the existing backslashes are unnecessary, or
> if you forgot to use them in the new text.

They are necessary inside single-quotes, but not inside backticks. IMHO
this entire file should be using backticks, but I didn't want to
reformat the entire file (and so I tried to at least keep the heading in
the same style as the rest of it).

> > +static char *tracking_ref_for(struct remote *remote, const char *refname)
> > +{
> > +	char *ret;
> > +
> > +	ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
> > +	if (!ret)
> > +		die(_("@{push} has no local tracking branch for remote '%s'"),
> > +		    refname);
> 
> I would imagine that it would be very plausible that anybody with a
> specific remote and the name of the ref that appears on that remote
> would want to learn the local name of the remote-tracking ref we use
> to track it.

I am not sure I understand. We do _not_ have a local name we use to
track it. That is the error. I can print "remote %s does not have branch
%s", if that is what you mean.

> But the error message limits the callers only to those who are
> involved in @{push} codepath.  Shouldn't the error check be done in
> the caller instead, anticipating the day this useful function ceases
> to be static?

Is it really a useful general function? If you remove the die() message,
it is literally a one-liner. My purpose in pulling it out at all was not
to repeat the die() message over and over in get_push_branch().

> I would suspect that such a change would make it just a one-liner,
> but I think this helper that takes remote and their refname is much
> easier to read than four inlined calls to apply_refspecs() that have
> to spell out remote->fetch, remote->fetch_refspec_nr separately.
> 
> Perhaps we would want 
> 
> 	struct refspecs {
>         	int nr, alloc;
>                 const char **refspec;
> 	} fetch_refspec;
> 
> in "struct remote", instead of these two separate fields, and then
> make apply_refspecs() take "struct refspecs *"?  I haven't checked
> and thought enough to decide if we want "struct refspec *" also in
> that new struct, though.

I think it is more complicated, as there are actually two arrays indexed
by each {fetch,push}_refspec_nr. We have "fetch_respec", which contains
the text (I assume), and then the "struct refspec". So ideally those
would be stored together in a single list, but of course many helper
functions want just the "struct refspec" list. So you still end up with
two lists, but just pushed down into a single struct. I guess that's
better, but I was trying to find a bound to my refactoring rather than
touching all of the code. :-/

-Peff

Re: [PATCH 6/6] sha1_name: implement @{push} shorthand

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +static char *tracking_ref_for(struct remote *remote, const char *refname)
>> > +{
>> > +	char *ret;
>> > +
>> > +	ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
>> > +	if (!ret)
>> > +		die(_("@{push} has no local tracking branch for remote '%s'"),
>> > +		    refname);
>> 
>> I would imagine that it would be very plausible that anybody with a
>> specific remote and the name of the ref that appears on that remote
>> would want to learn the local name of the remote-tracking ref we use
>> to track it.
>
> I am not sure I understand. We do _not_ have a local name we use to
> track it. That is the error. I can print "remote %s does not have branch
> %s", if that is what you mean.

No, I am not saying we should not detect an error.

>> But the error message limits the callers only to those who are
>> involved in @{push} codepath.  Shouldn't the error check be done in
>> the caller instead, anticipating the day this useful function ceases
>> to be static?
>
> Is it really a useful general function?

I often interact with this remote called 'ko'.  Do I have remote-tracking
branch to keep track of its 'master' branch, and if so, what is it?

Isn't that the question that the apply_refspecs() is answering?

> If you remove the die() message,
> it is literally a one-liner.

Yes, I thought I said that here...

>> I would suspect that such a change would make it just a one-liner,
>> but I think this helper that takes remote and their refname is much
>> easier to read than four inlined calls to apply_refspecs() that have
>> to spell out remote->fetch, remote->fetch_refspec_nr separately.

>> Perhaps we would want 
>> 
>> 	struct refspecs {
>>         	int nr, alloc;
>>                 const char **refspec;
>> 	} fetch_refspec;
>> 
>> in "struct remote", instead of these two separate fields, and then
>> make apply_refspecs() take "struct refspecs *"?  I haven't checked
>> and thought enough to decide if we want "struct refspec *" also in
>> that new struct, though.
>
> I think it is more complicated, as there are actually two arrays indexed
> by each {fetch,push}_refspec_nr. We have "fetch_respec", which contains
> the text (I assume), and then the "struct refspec".

Yeah, I thought I said that, too ;-)

> So ideally those
> would be stored together in a single list, but of course many helper
> functions want just the "struct refspec" list. So you still end up with
> two lists, but just pushed down into a single struct. I guess that's
> better, but I was trying to find a bound to my refactoring rather than
> touching all of the code. :-/

OK.

Re: [PATCH 6/6] sha1_name: implement @{push} shorthand

[Eric Sunshine]

On Tue, Mar 31, 2015 at 1:38 PM, Jeff King <peff@peff.net> wrote:
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 0796118..5d9df25 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.
>    `branch.<name>.merge`).  A missing branchname defaults to the
>    current one.
>
> +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> +  The suffix `@{push}` reports the branch "where we would push to" if
> +  `git push` were run while `branchname` was checked out (or the current
> +  `HEAD` is no branchname is specified). Since our push destination is

s/is no/if no/

> +  in a remote repository, of course, we report the local tracking branch
> +  that corresponds to that branch (i.e., something in `refs/remotes/`).
> ++
> +Here's an example to make it more clear:
> ++
> +------------------------------
> +$ git config push.default current
> +$ git config remote.pushdefault myfork
> +$ git checkout -b mybranch origin/master
> +
> +$ git rev-parse --symbolic-full-name @{upstream}
> +refs/remotes/origin/master
> +
> +$ git rev-parse --symbolic-full-name @{push}
> +refs/remotes/myfork/mybranch
> +------------------------------
> ++
> +Note in the example that we set up a triangular workflow, where we pull
> +from one location and push to another. In a non-triangular workflow,
> +`@{push}` is the same as `@{upstream}`, and there is no need for it.
> +
>  '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
>    A suffix '{caret}' to a revision parameter means the first parent of
>    that commit object.  '{caret}<n>' means the <n>th parent (i.e.

Re: [PATCH 6/6] sha1_name: implement @{push} shorthand

[Jeff King]

On Tue, Mar 31, 2015 at 05:41:02PM -0400, Eric Sunshine wrote:

> On Tue, Mar 31, 2015 at 1:38 PM, Jeff King <peff@peff.net> wrote:
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> > index 0796118..5d9df25 100644
> > --- a/Documentation/revisions.txt
> > +++ b/Documentation/revisions.txt
> > @@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.
> >    `branch.<name>.merge`).  A missing branchname defaults to the
> >    current one.
> >
> > +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> > +  The suffix `@{push}` reports the branch "where we would push to" if
> > +  `git push` were run while `branchname` was checked out (or the current
> > +  `HEAD` is no branchname is specified). Since our push destination is
> 
> s/is no/if no/

Thanks, fixed for my re-roll.

-Peff