[PATCH v2 0/12] implement @{push} shorthand

[PATCH v2 0/12] implement @{push} shorthand

[Jeff King]

This is a re-roll of the series at:

  http://thread.gmane.org/gmane.comp.version-control.git/266532

There were some minor fixes in response to review, but the main change
here is support for "git for-each-ref --format=%(push)". To do that, I
pulled the push logic into remote.[ch], where it can be used from both
sha1_name.c and for-each-ref.c. This is a better place for it to reside,
anyway, and may help in the future unifying it with the other remote
code that is used by `git push`.

In an effort to reuse as much of the @{upstream} code as possible, I did
similar refactoring for that side; we now have branch_get_upstream().
Even though the logic for getting @{upstream} isn't nearly as
complicated as for @{push}, I think several call-sites are improved by
using the new helper.

  [01/12]: remote.c: drop default_remote_name variable
  [02/12]: remote.c: drop "remote" pointer from "struct branch"
  [03/12]: remote.c: hoist branch.*.remote lookup out of remote_get_1
  [04/12]: remote.c: provide per-branch pushremote name
  [05/12]: remote.c: introduce branch_get_upstream helper
  [06/12]: remote.c: report specific errors from branch_get_upstream
  [07/12]: remote.c: add branch_get_push
  [08/12]: sha1_name: refactor upstream_mark
  [09/12]: sha1_name: refactor interpret_upstream_mark
  [10/12]: sha1_name: implement @{push} shorthand
  [11/12]: for-each-ref: use skip_prefix instead of starts_with
  [12/12]: for-each-ref: accept "%(push)" format

-Peff

[PATCH 01/12] 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 for 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>
---
Same as v1.

 remote.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index 68901b0..bec8b31 100644
--- a/remote.c
+++ b/remote.c
@@ -49,10 +49,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 +365,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))
@@ -501,12 +494,15 @@ static void alias_all_urls(void)
 
 static void read_config(void)
 {
+	static int loaded;
 	unsigned char sha1[20];
 	const char *head_ref;
 	int flag;
-	if (default_remote_name) /* did this already */
+
+	if (loaded)
 		return;
-	default_remote_name = "origin";
+	loaded = 1;
+
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -708,8 +704,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.rc3.477.gc25258d

[PATCH 02/12] 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 (with a "struct remote") based on that
name. However, it turns out that nobody really cares about
the latter field. The only two sites that access it at all
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" in the
     "struct branch" for that; we can just look it up for
     the duration of the operation.

So there is no need to have both fields; they are redundant
with each other (the struct remote contains the name, or you
can look up the struct from the name). It would be nice to
simplify this, especially as we are going to add matching
pushremote config in a future patch (and it would be nice to
keep them consistent).

So which one do we keep and which one do we get rid of?

If we had a lot of callers accessing the struct, it would be
more efficient to keep it (since you have to do a lookup to
go from the name to the struct, but not vice versa). But we
don't have a lot of callers; we have exactly one, so
efficiency doesn't matter. We can decide this based on
simplicity and readability.

And the meaning of the struct value is somewhat unclear. Is
it always the remote matching remote_name? If remote_name is
NULL (i.e., no per-branch config), does the struct fall back
to the "origin" remote, or is it also NULL? These questions
will get even more tricky with pushremotes, whose fallback
behavior is more complicated. So let's just store the name,
which pretty clearly represents the branch.*.remote config.
Any lookup or fallback behavior can then be implemented in
helper functions.

Signed-off-by: Jeff King <peff@peff.net>
---
Versus v1, I tried to explain the rationale a bit better in the commit
message. The code is the same.

 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 bec8b31..c85ecef 100644
--- a/remote.c
+++ b/remote.c
@@ -1632,15 +1632,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]),
@@ -1660,11 +1665,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.rc3.477.gc25258d

Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"

[Eric Sunshine]

On Fri, May 1, 2015 at 6:45 PM, Jeff King <peff@peff.net> wrote:
> When we create each branch struct, we fill in the
> "remote_name" field from the config, and then fill in the
> actual "remote" field (with a "struct remote") based on that
> name. However, it turns out that nobody really cares about
> the latter field. The only two sites that access it at all
> 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" in the
>      "struct branch" for that; we can just look it up for
>      the duration of the operation.
>
> So there is no need to have both fields; they are redundant
> with each other (the struct remote contains the name, or you
> can look up the struct from the name). It would be nice to
> simplify this, especially as we are going to add matching
> pushremote config in a future patch (and it would be nice to
> keep them consistent).
> [...]

Nice clear explanation, but...

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/remote.c b/remote.c
> index bec8b31..c85ecef 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1632,15 +1632,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]),
> @@ -1660,11 +1665,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);

When reading the actual patch, I was surprised to see unmentioned
changes to the reg->merge_nr check. Although the merge_nr
simplification seems sensible, it appears to be unrelated to the
stated purpose of the patch, and made the review more difficult since
it required keeping track of two distinct (yet textually intertwined)
changes. I wonder if it would make more sense to apply the merge_nr
simplification as a separate preparatory patch?

>         return ret;
>  }

Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"

[Jeff King]

On Sat, May 02, 2015 at 11:34:25PM -0400, Eric Sunshine wrote:

> > -       if (ret && ret->remote_name) {
> > -               ret->remote = remote_get(ret->remote_name);
> > -               if (ret->merge_nr)
> > -                       set_merge(ret);
> > -       }
> > +       if (ret)
> > +              set_merge(ret);
> 
> When reading the actual patch, I was surprised to see unmentioned
> changes to the reg->merge_nr check. Although the merge_nr
> simplification seems sensible, it appears to be unrelated to the
> stated purpose of the patch, and made the review more difficult since
> it required keeping track of two distinct (yet textually intertwined)
> changes. I wonder if it would make more sense to apply the merge_nr
> simplification as a separate preparatory patch?

I didn't actually mean to change any behavior with respect to
ret->merge_nr here (and I don't think I did). What I did was blindly
move everything in the conditional after the remote_get into set_merge,
so that it happened in the same order (and the remote_get moves into
set_merge, because we no longer have the struct element that it was
formerly passed down in).

But actually, ret->merge_nr comes from make_branch, and we could
continue to respect it regardless of the remote values (i.e., they are
both preconditions to setting up the merge data, but it doesn't matter
in which order we check them).

One thing I did notice while looking at this is that it seems like we
may leak if you call branch_get multiple times. The make_branch()
function may sometimes return a brand-new branch and sometimes return a
cached version from the "branches" array. In the latter case, we
continue to update the "remote" pointer (which is wasteful but at least
does not leak because the remotes themselves are part of a cached list).
But then we will repeatedly re-allocate the ret->merge array. We
probably should make sure it is NULL before trying to fill it in.

I'll see if I can insert a cleanup patch in this part of the series.

-Peff

Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"

[Jeff King]

On Tue, May 05, 2015 at 03:31:05PM -0400, Jeff King wrote:

> One thing I did notice while looking at this is that it seems like we
> may leak if you call branch_get multiple times. The make_branch()
> function may sometimes return a brand-new branch and sometimes return a
> cached version from the "branches" array. In the latter case, we
> continue to update the "remote" pointer (which is wasteful but at least
> does not leak because the remotes themselves are part of a cached list).
> But then we will repeatedly re-allocate the ret->merge array. We
> probably should make sure it is NULL before trying to fill it in.
> 
> I'll see if I can insert a cleanup patch in this part of the series.

Here's what I came up with. I'm sending it separately in case you have
any early comments, but it will be part of the next re-roll (just before
the existing patch 2 we're discussing here).

-- >8 --
Subject: remote.c: refactor setup of branch->merge list

When we call branch_get() to lookup or create a "struct
branch", we make sure the "merge" field is filled in so that
callers can access it. But the conditions under which we do
so are a little confusing, and can lead to two funny
situations:

  1. If there's no branch.*.remote config, we cannot provide
     branch->merge (because it is really just an application
     of branch.*.merge to our remote's refspecs). But
     branch->merge_nr may be non-zero, leading callers to be
     believe they can access branch->merge (e.g., in
     branch_merge_matches and elsewhere).

     It doesn't look like this can cause a segfault in
     practice, as most code paths dealing with merge config
     will bail early if there is no remote defined. But it's
     a bit of a dangerous construct.

     We can fix this by setting merge_nr to "0" explicitly
     when we realize that we have no merge config. Note that
     merge_nr also counts the "merge_name" fields (which we
     _do_ have; that's how merge_nr got incremented), so we
     will "lose" access to them, in the sense that we forget
     how many we had. But no callers actually care; we use
     merge_name only while iteratively reading the config,
     and then convert it to the final "merge" form the first
     time somebody calls branch_get().

  2. We set up the "merge" field every time branch_get is
     called, even if it has already been done. This leaks
     memory.

     It's not a big deal in practice, since most code paths
     will access only one branch, or perhaps each branch
     only one time. But if you want to be pathological, you
     can leak arbitrary memory with:

       yes @{upstream} | head -1000 | git rev-list --stdin

     We can fix this by skipping setup when branch->merge is
     already non-NULL.

In addition to those two fixes, this patch pushes the "do we
need to setup merge?" logic down into set_merge, where it is
a bit easier to follow.

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

diff --git a/remote.c b/remote.c
index bec8b31..ac17e66 100644
--- a/remote.c
+++ b/remote.c
@@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret)
 	unsigned char sha1[20];
 	int i;
 
+	if (!ret)
+		return; /* no branch */
+	if (ret->merge)
+		return; /* already run */
+	if (!ret->remote_name || !ret->merge_nr) {
+		/*
+		 * no merge config; let's make sure we don't confuse callers
+		 * with a non-zero merge_nr but a NULL merge
+		 */
+		ret->merge_nr = 0;
+		return;
+	}
+
 	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));
@@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
-	if (ret && ret->remote_name) {
+	if (ret && ret->remote_name)
 		ret->remote = remote_get(ret->remote_name);
-		if (ret->merge_nr)
-			set_merge(ret);
-	}
+	set_merge(ret);
 	return ret;
 }
 
-- 
2.4.0.488.gf55b16a

[PATCH 03/12] 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>
---
Same as v1.

 remote.c | 21 ++++++++++++++-------
 remote.h |  1 +
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index c85ecef..a27f795 100644
--- a/remote.c
+++ b/remote.c
@@ -692,6 +692,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;
@@ -703,13 +715,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.rc3.477.gc25258d

[PATCH 04/12] 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>
---
Versus v1, I did something a little clever by passing a function pointer
around (versus a flag and letting the caller do a conditional based on
the flag). Too clever?

 remote.c | 40 ++++++++++++++++++++++------------------
 remote.h |  2 ++
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/remote.c b/remote.c
index a27f795..9f84ea3 100644
--- a/remote.c
+++ b/remote.c
@@ -49,7 +49,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;
@@ -367,9 +366,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);
@@ -510,10 +507,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();
 }
 
@@ -704,20 +697,31 @@ 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,
+				   const char *(*get_default)(struct branch *, int *))
 {
 	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 = get_default(current_branch, &name_given);
 
 	ret = make_remote(name, 0);
 	if (valid_remote_nick(name)) {
@@ -738,13 +742,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, remote_for_branch);
 }
 
 struct remote *pushremote_get(const char *name)
 {
 	read_config();
-	return remote_get_1(name, pushremote_name);
+	return remote_get_1(name, pushremote_for_branch);
 }
 
 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.rc3.477.gc25258d

Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

[Eric Sunshine]

On Fri, May 1, 2015 at 6:46 PM, Jeff King <peff@peff.net> wrote:
> 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>
> ---
> Versus v1, I did something a little clever by passing a function pointer
> around (versus a flag and letting the caller do a conditional based on
> the flag). Too clever?

FWIW: I found this "clever" version easy enough to follow.

However, if you push a tiny bit of the work into the callers of
remote_get_1(), then you can do away with the "cleverness" altogether,
can't you? Something like this:

    static struct remote_get_1(const char *name, int explicit)
    {
        struct remote *ret = make_remote(name, 0);
        ...
        if (explicit && valid_remote(ret))
            ...
        ...
    }

    struct remote *remote_get(const char *name)
    {
        int explicit = !!name;
        read_config();
        if (!name)
            name = remote_for_branch(current_branch, &explicit);
        return remote_get_1(name, explicit);
    }

    struct remote *pushremote_get(const char *name)
    {
        int explicit = !!name;
        read_config();
        if (!name)
            name = pushremote_for_branch(current_branch, &explicit);
        return remote_get_1(name, explicit);
    }

> diff --git a/remote.c b/remote.c
> index a27f795..9f84ea3 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -704,20 +697,31 @@ 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,
> +                                  const char *(*get_default)(struct branch *, int *))
>  {
>         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 = get_default(current_branch, &name_given);
>
>         ret = make_remote(name, 0);
>         if (valid_remote_nick(name)) {
> @@ -738,13 +742,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, remote_for_branch);
>  }
>
>  struct remote *pushremote_get(const char *name)
>  {
>         read_config();
> -       return remote_get_1(name, pushremote_name);
> +       return remote_get_1(name, pushremote_for_branch);
>  }
>
>  int remote_is_configured(const char *name)

Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

[Jeff King]

On Sun, May 03, 2015 at 12:51:13AM -0400, Eric Sunshine wrote:

> > Versus v1, I did something a little clever by passing a function pointer
> > around (versus a flag and letting the caller do a conditional based on
> > the flag). Too clever?
> 
> FWIW: I found this "clever" version easy enough to follow.
> 
> However, if you push a tiny bit of the work into the callers of
> remote_get_1(), then you can do away with the "cleverness" altogether,
> can't you? Something like this:

Yeah, it's just that it goes in the opposite direction I was trying for,
which is to have as little code as possible in the wrapper functions (in
fact, I think after my changes you could even bump the read_config()
call into remote_get_1; before my changes, it depended on the pushremote
config being set before the call).

I agree it is not so much code, though, and maybe it makes the flow a
little clearer. I'll play with it for the re-roll.

-Peff

Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

[Eric Sunshine]

On Tue, May 5, 2015 at 3:33 PM, Jeff King <peff@peff.net> wrote:
> On Sun, May 03, 2015 at 12:51:13AM -0400, Eric Sunshine wrote:
>
>> > Versus v1, I did something a little clever by passing a function pointer
>> > around (versus a flag and letting the caller do a conditional based on
>> > the flag). Too clever?
>>
>> FWIW: I found this "clever" version easy enough to follow.
>>
>> However, if you push a tiny bit of the work into the callers of
>> remote_get_1(), then you can do away with the "cleverness" altogether,
>> can't you? Something like this:
>
> Yeah, it's just that it goes in the opposite direction I was trying for,
> which is to have as little code as possible in the wrapper functions (in
> fact, I think after my changes you could even bump the read_config()
> call into remote_get_1; before my changes, it depended on the pushremote
> config being set before the call).

I also noticed that read_config() could be moved into remote_get_1().
In fact, with that change, then the wrappers really do collapse nicely
to 1-liners, so the "clever" function pointer approach probably is
cleaner; and it's nicely generalized over the previous round with the
boolean argument to remote_get_1().

Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

[Jeff King]

On Tue, May 05, 2015 at 03:48:29PM -0400, Eric Sunshine wrote:

> > Yeah, it's just that it goes in the opposite direction I was trying for,
> > which is to have as little code as possible in the wrapper functions (in
> > fact, I think after my changes you could even bump the read_config()
> > call into remote_get_1; before my changes, it depended on the pushremote
> > config being set before the call).
> 
> I also noticed that read_config() could be moved into remote_get_1().
> In fact, with that change, then the wrappers really do collapse nicely
> to 1-liners, so the "clever" function pointer approach probably is
> cleaner; and it's nicely generalized over the previous round with the
> boolean argument to remote_get_1().

I ended up with this patch, which will go right after the one we're
discussing:

-- >8 --
Subject: remote.c: hoist read_config into remote_get_1

Before the previous commit, we had to make sure that
read_config() was called before entering remote_get_1,
because we needed to pass pushremote_name by value. But now
that we pass a function, we can let remote_get_1 handle
loading the config itself, turning our wrappers into true
one-liners.

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

diff --git a/remote.c b/remote.c
index a91d063..e6b29b3 100644
--- a/remote.c
+++ b/remote.c
@@ -718,6 +718,8 @@ static struct remote *remote_get_1(const char *name,
 	struct remote *ret;
 	int name_given = 0;
 
+	read_config();
+
 	if (name)
 		name_given = 1;
 	else
@@ -741,13 +743,11 @@ static struct remote *remote_get_1(const char *name,
 
 struct remote *remote_get(const char *name)
 {
-	read_config();
 	return remote_get_1(name, remote_for_branch);
 }
 
 struct remote *pushremote_get(const char *name)
 {
-	read_config();
 	return remote_get_1(name, pushremote_for_branch);
 }
 
-- 
2.4.0.488.gf55b16a

Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

[Eric Sunshine]

On Thu, May 7, 2015 at 5:38 AM, Jeff King <peff@peff.net> wrote:
> On Tue, May 05, 2015 at 03:48:29PM -0400, Eric Sunshine wrote:
>
>> > Yeah, it's just that it goes in the opposite direction I was trying for,
>> > which is to have as little code as possible in the wrapper functions (in
>> > fact, I think after my changes you could even bump the read_config()
>> > call into remote_get_1; before my changes, it depended on the pushremote
>> > config being set before the call).
>>
>> I also noticed that read_config() could be moved into remote_get_1().
>> In fact, with that change, then the wrappers really do collapse nicely
>> to 1-liners, so the "clever" function pointer approach probably is
>> cleaner; and it's nicely generalized over the previous round with the
>> boolean argument to remote_get_1().
>
> I ended up with this patch, which will go right after the one we're
> discussing:

Nice, I like it.

> -- >8 --
> Subject: remote.c: hoist read_config into remote_get_1
>
> Before the previous commit, we had to make sure that
> read_config() was called before entering remote_get_1,
> because we needed to pass pushremote_name by value. But now
> that we pass a function, we can let remote_get_1 handle
> loading the config itself, turning our wrappers into true
> one-liners.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  remote.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index a91d063..e6b29b3 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -718,6 +718,8 @@ static struct remote *remote_get_1(const char *name,
>         struct remote *ret;
>         int name_given = 0;
>
> +       read_config();
> +
>         if (name)
>                 name_given = 1;
>         else
> @@ -741,13 +743,11 @@ static struct remote *remote_get_1(const char *name,
>
>  struct remote *remote_get(const char *name)
>  {
> -       read_config();
>         return remote_get_1(name, remote_for_branch);
>  }
>
>  struct remote *pushremote_get(const char *name)
>  {
> -       read_config();
>         return remote_get_1(name, pushremote_for_branch);
>  }
>
> --
> 2.4.0.488.gf55b16a

[PATCH 05/12] remote.c: introduce branch_get_upstream helper

[Jeff King]

All of the information needed to find the @{upstream} of a
branch is included in the branch struct, but callers have to
navigate a series of possible-NULL values to get there.
Let's wrap that logic up in an easy-to-read helper.

Signed-off-by: Jeff King <peff@peff.net>
---
New in v2.

 builtin/branch.c       |  8 +++-----
 builtin/for-each-ref.c |  5 ++---
 builtin/log.c          |  7 ++-----
 remote.c               | 12 +++++++++---
 remote.h               |  7 +++++++
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 1d15037..bd1fa0b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -123,14 +123,12 @@ static int branch_merged(int kind, const char *name,
 
 	if (kind == REF_LOCAL_BRANCH) {
 		struct branch *branch = branch_get(name);
+		const char *upstream = branch_get_upstream(branch);
 		unsigned char sha1[20];
 
-		if (branch &&
-		    branch->merge &&
-		    branch->merge[0] &&
-		    branch->merge[0]->dst &&
+		if (upstream &&
 		    (reference_name = reference_name_to_free =
-		     resolve_refdup(branch->merge[0]->dst, RESOLVE_REF_READING,
+		     resolve_refdup(upstream, RESOLVE_REF_READING,
 				    sha1, NULL)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
 	}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 83f9cf9..dc2a201 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -664,10 +664,9 @@ static void populate_value(struct refinfo *ref)
 				continue;
 			branch = branch_get(ref->refname + 11);
 
-			if (!branch || !branch->merge || !branch->merge[0] ||
-			    !branch->merge[0]->dst)
+			refname = branch_get_upstream(branch);
+			if (!refname)
 				continue;
-			refname = branch->merge[0]->dst;
 		} else if (starts_with(name, "color:")) {
 			char color[COLOR_MAXLEN] = "";
 
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..fb61c08 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1632,16 +1632,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		break;
 	default:
 		current_branch = branch_get(NULL);
-		if (!current_branch || !current_branch->merge
-					|| !current_branch->merge[0]
-					|| !current_branch->merge[0]->dst) {
+		upstream = branch_get_upstream(current_branch);
+		if (!upstream) {
 			fprintf(stderr, _("Could not find a tracked"
 					" remote branch, please"
 					" specify <upstream> manually.\n"));
 			usage_with_options(cherry_usage, options);
 		}
-
-		upstream = current_branch->merge[0]->dst;
 	}
 
 	init_revisions(&revs, prefix);
diff --git a/remote.c b/remote.c
index 9f84ea3..c826fad 100644
--- a/remote.c
+++ b/remote.c
@@ -1695,6 +1695,13 @@ int branch_merge_matches(struct branch *branch,
 	return refname_match(branch->merge[i]->src, refname);
 }
 
+const char *branch_get_upstream(struct branch *branch)
+{
+	if (!branch || !branch->merge || !branch->merge[0])
+		return NULL;
+	return branch->merge[0]->dst;
+}
+
 static int ignore_symref_update(const char *refname)
 {
 	unsigned char sha1[20];
@@ -1904,12 +1911,11 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
 	int rev_argc;
 
 	/* Cannot stat unless we are marked to build on top of somebody else. */
-	if (!branch ||
-	    !branch->merge || !branch->merge[0] || !branch->merge[0]->dst)
+	base = branch_get_upstream(branch);
+	if (!base)
 		return 0;
 
 	/* Cannot stat if what we used to build on no longer exists */
-	base = branch->merge[0]->dst;
 	if (read_ref(base, sha1))
 		return -1;
 	theirs = lookup_commit_reference(sha1);
diff --git a/remote.h b/remote.h
index 30a11da..d968952 100644
--- a/remote.h
+++ b/remote.h
@@ -218,6 +218,13 @@ 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 *);
 
+/**
+ * Return the fully-qualified refname of the tracking branch for `branch`.
+ * I.e., what "branch@{upstream}" would give you. Returns NULL if no
+ * upstream is defined.
+ */
+const char *branch_get_upstream(struct branch *branch);
+
 /* Flags to match_refs. */
 enum match_refs_flags {
 	MATCH_REFS_NONE		= 0,
-- 
2.4.0.rc3.477.gc25258d

[PATCH 06/12] remote.c: report specific errors from branch_get_upstream

[Jeff King]

When the previous commit introduced the branch_get_upstream
helper, there was one call-site that could not be converted:
the one in sha1_name.c, which gives detailed error messages
for each possible failure.

Let's teach the helper to optionally report these specific
errors. This lets us convert another callsite, and means we
can use the helper in other locations that want to give the
same error messages.

The logic and error messages come straight from sha1_name.c,
with the exception that we start each error with a lowercase
letter, as is our usual style (note that a few tests need
updated as a result).

Signed-off-by: Jeff King <peff@peff.net>
---
So this uses the recently discussed "pass the error back in a strbuf"
technique. We don't need an integer flag here because nobody cares about
the reason; only whether to die and report it (so a "_gently" form would
also work).

This adds an extra NULL parameter to every call-site of
branch_get_upstream (in the _gently paradigm, we'd pass an extra "0"
flag). I waffled on just leaving that as-is, and implementing it as a
wrapper to a new branch_get_upstream_err.

Overall, I think it makes the flow work pretty well, and the allocation
issues are non-existent because the callers all either want to die with
the error or ignore it. As an aside, while thinking about this I
recalled that one of the complaints against using a statically-sized
buffer for error messages is that we didn't want to truncate them. But
note that calling die() will end up in vreportf, which will itself
truncate.

 builtin/branch.c              |  2 +-
 builtin/for-each-ref.c        |  2 +-
 builtin/log.c                 |  2 +-
 remote.c                      | 33 +++++++++++++++++++++++++++++----
 remote.h                      |  6 +++++-
 sha1_name.c                   | 25 +++++++------------------
 t/t1507-rev-parse-upstream.sh |  8 ++++----
 7 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index bd1fa0b..b7202b3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -123,7 +123,7 @@ static int branch_merged(int kind, const char *name,
 
 	if (kind == REF_LOCAL_BRANCH) {
 		struct branch *branch = branch_get(name);
-		const char *upstream = branch_get_upstream(branch);
+		const char *upstream = branch_get_upstream(branch, NULL);
 		unsigned char sha1[20];
 
 		if (upstream &&
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index dc2a201..18d209b 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -664,7 +664,7 @@ static void populate_value(struct refinfo *ref)
 				continue;
 			branch = branch_get(ref->refname + 11);
 
-			refname = branch_get_upstream(branch);
+			refname = branch_get_upstream(branch, NULL);
 			if (!refname)
 				continue;
 		} else if (starts_with(name, "color:")) {
diff --git a/builtin/log.c b/builtin/log.c
index fb61c08..6faeb82 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1632,7 +1632,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		break;
 	default:
 		current_branch = branch_get(NULL);
-		upstream = branch_get_upstream(current_branch);
+		upstream = branch_get_upstream(current_branch, NULL);
 		if (!upstream) {
 			fprintf(stderr, _("Could not find a tracked"
 					" remote branch, please"
diff --git a/remote.c b/remote.c
index c826fad..146b94d 100644
--- a/remote.c
+++ b/remote.c
@@ -1695,10 +1695,35 @@ int branch_merge_matches(struct branch *branch,
 	return refname_match(branch->merge[i]->src, refname);
 }
 
-const char *branch_get_upstream(struct branch *branch)
+__attribute((format (printf,2,3)))
+static const char *error_buf(struct strbuf *err, const char *fmt, ...)
 {
-	if (!branch || !branch->merge || !branch->merge[0])
-		return NULL;
+	if (err) {
+		va_list ap;
+		va_start(ap, fmt);
+		strbuf_vaddf(err, fmt, ap);
+		va_end(ap);
+	}
+	return NULL;
+}
+
+const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
+{
+	if (!branch)
+		return error_buf(err, _("HEAD does not point to a branch"));
+	if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {
+		if (!ref_exists(branch->refname))
+			return error_buf(err, _("no such branch: '%s'"),
+					 branch->name);
+		if (!branch->merge)
+			return error_buf(err,
+					 _("no upstream configured for branch '%s'"),
+					 branch->name);
+		return error_buf(err,
+				 _("upstream branch '%s' not stored as a remote-tracking branch"),
+				 branch->merge[0]->src);
+	}
+
 	return branch->merge[0]->dst;
 }
 
@@ -1911,7 +1936,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
 	int rev_argc;
 
 	/* Cannot stat unless we are marked to build on top of somebody else. */
-	base = branch_get_upstream(branch);
+	base = branch_get_upstream(branch, NULL);
 	if (!base)
 		return 0;
 
diff --git a/remote.h b/remote.h
index d968952..03ca005 100644
--- a/remote.h
+++ b/remote.h
@@ -222,8 +222,12 @@ int branch_merge_matches(struct branch *, int n, const char *);
  * Return the fully-qualified refname of the tracking branch for `branch`.
  * I.e., what "branch@{upstream}" would give you. Returns NULL if no
  * upstream is defined.
+ *
+ * If `err` is not NULL and no upstream is defined, a more specific error
+ * message is recorded there (if the function does not return NULL, then
+ * `err` is not touched).
  */
-const char *branch_get_upstream(struct branch *branch);
+const char *branch_get_upstream(struct branch *branch, struct strbuf *err);
 
 /* Flags to match_refs. */
 enum match_refs_flags {
diff --git a/sha1_name.c b/sha1_name.c
index 6d10f05..461157a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1059,27 +1059,16 @@ static const char *get_upstream_branch(const char *branch_buf, int len)
 {
 	char *branch = xstrndup(branch_buf, len);
 	struct branch *upstream = branch_get(*branch ? branch : NULL);
+	struct strbuf err = STRBUF_INIT;
+	const char *ret;
 
-	/*
-	 * Upstream can be NULL only if branch refers to HEAD and HEAD
-	 * points to something different than a branch.
-	 */
-	if (!upstream)
-		die(_("HEAD does not point to a branch"));
-	if (!upstream->merge || !upstream->merge[0]->dst) {
-		if (!ref_exists(upstream->refname))
-			die(_("No such branch: '%s'"), branch);
-		if (!upstream->merge) {
-			die(_("No upstream configured for branch '%s'"),
-				upstream->name);
-		}
-		die(
-			_("Upstream branch '%s' not stored as a remote-tracking branch"),
-			upstream->merge[0]->src);
-	}
 	free(branch);
 
-	return upstream->merge[0]->dst;
+	ret = branch_get_upstream(upstream, &err);
+	if (!ret)
+		die("%s", err.buf);
+
+	return ret;
 }
 
 static int interpret_upstream_mark(const char *name, int namelen,
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 1978947..46ef1f2 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -150,7 +150,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '
 
 test_expect_success 'branch@{u} error message when no upstream' '
 	cat >expect <<-EOF &&
-	fatal: No upstream configured for branch ${sq}non-tracking${sq}
+	fatal: no upstream configured for branch ${sq}non-tracking${sq}
 	EOF
 	error_message non-tracking@{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -158,7 +158,7 @@ test_expect_success 'branch@{u} error message when no upstream' '
 
 test_expect_success '@{u} error message when no upstream' '
 	cat >expect <<-EOF &&
-	fatal: No upstream configured for branch ${sq}master${sq}
+	fatal: no upstream configured for branch ${sq}master${sq}
 	EOF
 	test_must_fail git rev-parse --verify @{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -166,7 +166,7 @@ test_expect_success '@{u} error message when no upstream' '
 
 test_expect_success 'branch@{u} error message with misspelt branch' '
 	cat >expect <<-EOF &&
-	fatal: No such branch: ${sq}no-such-branch${sq}
+	fatal: no such branch: ${sq}no-such-branch${sq}
 	EOF
 	error_message no-such-branch@{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -183,7 +183,7 @@ test_expect_success '@{u} error message when not on a branch' '
 
 test_expect_success 'branch@{u} error message if upstream branch not fetched' '
 	cat >expect <<-EOF &&
-	fatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
+	fatal: upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
 	EOF
 	error_message bad-upstream@{u} 2>actual &&
 	test_i18ncmp expect actual
-- 
2.4.0.rc3.477.gc25258d

[PATCH 07/12] remote.c: add branch_get_push

[Jeff King]

In a triangular workflow, the place you pull from and the
place you push to may be different. As we have
branch_get_upstream for the former, this patch adds
branch_get_push for the latter (and as the former implements
@{upstream}, so will this implement @{push} in a future
patch).

Note that the memory-handling for the return value bears
some explanation. Some code paths require allocating a new
string, and some let us return an existing string. We should
provide a consistent interface to the caller, so it knows
whether to free the result or not.

We could do so by xstrdup-ing any existing strings, and
having the caller always free. But that makes us
inconsistent with branch_get_upstream, so we would prefer to
simply take ownership of the resulting string. We do so by
storing it inside the "struct branch", just as we do with
the upstream refname (in that case we compute it when the
branch is created, but there's no reason not to just fill
it in lazily in this case).

Signed-off-by: Jeff King <peff@peff.net>
---
This patch is new in v2, but the logic is basically ripped from
sha1_name.c in v1.

 remote.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h | 10 ++++++++
 2 files changed, 95 insertions(+)

diff --git a/remote.c b/remote.c
index 146b94d..7f0fe28 100644
--- a/remote.c
+++ b/remote.c
@@ -1727,6 +1727,91 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 	return branch->merge[0]->dst;
 }
 
+static const char *tracking_for_push_dest(struct remote *remote,
+					  const char *refname,
+					  struct strbuf *err)
+{
+	char *ret;
+
+	ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
+	if (!ret)
+		return error_buf(err,
+				 _("push destination '%s' on remote '%s' has no local tracking branch"),
+				 refname, remote->name);
+	return ret;
+}
+
+static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
+{
+	struct remote *remote;
+
+	if (!branch)
+		return error_buf(err, _("HEAD does not point to a branch"));
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		return error_buf(err,
+				 _("branch '%s' has no remote for pushing"),
+				 branch->name);
+
+	if (remote->push_refspec_nr) {
+		char *dst;
+		const char *ret;
+
+		dst = apply_refspecs(remote->push, remote->push_refspec_nr,
+				     branch->refname);
+		if (!dst)
+			return error_buf(err,
+					 _("push refspecs for '%s' do not include '%s'"),
+					 remote->name, branch->name);
+
+		ret = tracking_for_push_dest(remote, dst, err);
+		free(dst);
+		return ret;
+	}
+
+	if (remote->mirror)
+		return tracking_for_push_dest(remote, branch->refname, err);
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		return error_buf(err, _("push has no destination (push.default is 'nothing')"));
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return tracking_for_push_dest(remote, branch->refname, err);
+
+	case PUSH_DEFAULT_UPSTREAM:
+		return branch_get_upstream(branch, err);
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up, *cur;
+
+			up = branch_get_upstream(branch, err);
+			if (!up)
+				return NULL;
+			cur = tracking_for_push_dest(remote, branch->refname, err);
+			if (!cur)
+				return NULL;
+			if (strcmp(cur, up))
+				return error_buf(err,
+						 _("cannot resolve 'simple' push to a single destination"));
+			return cur;
+		}
+	}
+
+	die("BUG: unhandled push situation");
+}
+
+const char *branch_get_push(struct branch *branch, struct strbuf *err)
+{
+	if (!branch->push_tracking_ref)
+		branch->push_tracking_ref = branch_get_push_1(branch, err);
+	return branch->push_tracking_ref;
+}
+
 static int ignore_symref_update(const char *refname)
 {
 	unsigned char sha1[20];
diff --git a/remote.h b/remote.h
index 03ca005..3326f2b 100644
--- a/remote.h
+++ b/remote.h
@@ -209,6 +209,8 @@ struct branch {
 	struct refspec **merge;
 	int merge_nr;
 	int merge_alloc;
+
+	const char *push_tracking_ref;
 };
 
 struct branch *branch_get(const char *name);
@@ -229,6 +231,14 @@ int branch_merge_matches(struct branch *, int n, const char *);
  */
 const char *branch_get_upstream(struct branch *branch, struct strbuf *err);
 
+/**
+ * Return the tracking branch that corresponds to the ref we would push to
+ * given a bare `git push` while `branch` is checked out.
+ *
+ * The return value and `err` conventions match those of `branch_get_upstream`.
+ */
+const char *branch_get_push(struct branch *branch, struct strbuf *err);
+
 /* Flags to match_refs. */
 enum match_refs_flags {
 	MATCH_REFS_NONE		= 0,
-- 
2.4.0.rc3.477.gc25258d

[PATCH 08/12] 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>
---
Same as v1.

 sha1_name.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 461157a..1005f45 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.rc3.477.gc25258d

[PATCH 09/12] sha1_name: refactor interpret_upstream_mark

[Jeff King]

Now that most of the logic for our local get_upstream_branch
has been pushed into the generic branch_get_upstream, we can
fold the remainder into interpret_upstream_mark.

Furthermore, what remains is generic to any branch-related
"@{foo}" we might add in the future, and there's enough
boilerplate that we'd like to reuse it. Let's parameterize
the two operations (parsing the mark and computing its
value) so that we can reuse this for "@{push}" in the near
future.

Signed-off-by: Jeff King <peff@peff.net>
---
More function pointer cleverness. Here it seems more justified to me,
because we may eventually grow the list of interpret_branch_mark()
users (e.g., for the concept discussed previously as @{publish}).

 sha1_name.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 1005f45..506e0c9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1061,35 +1061,36 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)
 	free(s);
 }
 
-static const char *get_upstream_branch(const char *branch_buf, int len)
-{
-	char *branch = xstrndup(branch_buf, len);
-	struct branch *upstream = branch_get(*branch ? branch : NULL);
-	struct strbuf err = STRBUF_INIT;
-	const char *ret;
-
-	free(branch);
-
-	ret = branch_get_upstream(upstream, &err);
-	if (!ret)
-		die("%s", err.buf);
-
-	return ret;
-}
-
-static int interpret_upstream_mark(const char *name, int namelen,
-				   int at, struct strbuf *buf)
+static int interpret_branch_mark(const char *name, int namelen,
+				 int at, struct strbuf *buf,
+				 int (*get_mark)(const char *, int),
+				 const char *(*get_data)(struct branch *,
+							 struct strbuf *))
 {
 	int len;
+	struct branch *branch;
+	struct strbuf err = STRBUF_INIT;
+	const char *value;
 
-	len = upstream_mark(name + at, namelen - at);
+	len = get_mark(name + at, namelen - at);
 	if (!len)
 		return -1;
 
 	if (memchr(name, ':', at))
 		return -1;
 
-	set_shortened_ref(buf, get_upstream_branch(name, at));
+	if (at) {
+		char *name_str = xmemdupz(name, at);
+		branch = branch_get(name_str);
+		free(name_str);
+	} else
+		branch = branch_get(NULL);
+
+	value = get_data(branch, &err);
+	if (!value)
+		die("%s", err.buf);
+
+	set_shortened_ref(buf, value);
 	return len + at;
 }
 
@@ -1140,7 +1141,8 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 		if (len > 0)
 			return reinterpret(name, namelen, len, buf);
 
-		len = interpret_upstream_mark(name, namelen, at - name, buf);
+		len = interpret_branch_mark(name, namelen, at - name, buf,
+					    upstream_mark, branch_get_upstream);
 		if (len > 0)
 			return len;
 	}
-- 
2.4.0.rc3.477.gc25258d

[PATCH 10/12] 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.

The heavy lifting is all done by branch_get_push; here we
just wire it up to the "@{push}" syntax.

Signed-off-by: Jeff King <peff@peff.net>
---
Most of this is from v1, but the heavy lifting was already extracted.

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

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 0796118..d85e303 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' if 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 506e0c9..1096943 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;
 				}
@@ -1145,6 +1152,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 					    upstream_mark, branch_get_upstream);
 		if (len > 0)
 			return len;
+
+		len = interpret_branch_mark(name, namelen, at - name, buf,
+					    push_mark, branch_get_push);
+		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.rc3.477.gc25258d

[PATCH 11/12] for-each-ref: use skip_prefix instead of starts_with

[Jeff King]

This saves us having to maintain a magic number to skip past
the matched prefix.

Signed-off-by: Jeff King <peff@peff.net>
---
Noticed because I'm adding similar code in the next patch...

 builtin/for-each-ref.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 18d209b..345d8dd 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -659,10 +659,12 @@ static void populate_value(struct refinfo *ref)
 		else if (starts_with(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (starts_with(name, "upstream")) {
+			const char *branch_name;
 			/* only local branches may have an upstream */
-			if (!starts_with(ref->refname, "refs/heads/"))
+			if (!skip_prefix(ref->refname, "refs/heads/",
+					 &branch_name))
 				continue;
-			branch = branch_get(ref->refname + 11);
+			branch = branch_get(branch_name);
 
 			refname = branch_get_upstream(branch, NULL);
 			if (!refname)
-- 
2.4.0.rc3.477.gc25258d

[PATCH 12/12] for-each-ref: accept "%(push)" format

[Jeff King]

Just as we have "%(upstream)" to report the "@{upstream}"
for each ref, this patch adds "%(push)" to match "@{push}".
It supports the same tracking format modifiers as upstream
(because you may want to know, for example, which branches
have commits to push).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-for-each-ref.txt |  6 ++++++
 builtin/for-each-ref.c             | 17 +++++++++++++++--
 t/t6300-for-each-ref.sh            | 13 ++++++++++++-
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 4240875..7f8d9a5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -97,6 +97,12 @@ upstream::
 	or "=" (in sync).  Has no effect if the ref does not have
 	tracking information associated with it.
 
+push::
+	The name of a local ref which represents the `@{push}` location
+	for the displayed ref. Respects `:short`, `:track`, and
+	`:trackshort` options as `upstream` does. Produces an empty
+	string if no `@{push}` ref is configured.
+
 HEAD::
 	'*' if HEAD matches current ref (the checked out branch), ' '
 	otherwise.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 345d8dd..6847400 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -74,6 +74,7 @@ static struct {
 	{ "contents:body" },
 	{ "contents:signature" },
 	{ "upstream" },
+	{ "push" },
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
@@ -669,6 +670,16 @@ static void populate_value(struct refinfo *ref)
 			refname = branch_get_upstream(branch, NULL);
 			if (!refname)
 				continue;
+		} else if (starts_with(name, "push")) {
+			const char *branch_name;
+			if (!skip_prefix(ref->refname, "refs/heads/",
+					 &branch_name))
+				continue;
+			branch = branch_get(branch_name);
+
+			refname = branch_get_push(branch, NULL);
+			if (!refname)
+				continue;
 		} else if (starts_with(name, "color:")) {
 			char color[COLOR_MAXLEN] = "";
 
@@ -714,7 +725,8 @@ static void populate_value(struct refinfo *ref)
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
 			else if (!strcmp(formatp, "track") &&
-				 starts_with(name, "upstream")) {
+				 (starts_with(name, "upstream") ||
+				  starts_with(name, "push"))) {
 				char buf[40];
 
 				if (stat_tracking_info(branch, &num_ours,
@@ -736,7 +748,8 @@ static void populate_value(struct refinfo *ref)
 				}
 				continue;
 			} else if (!strcmp(formatp, "trackshort") &&
-				   starts_with(name, "upstream")) {
+				   (starts_with(name, "upstream") ||
+				    starts_with(name, "push"))) {
 				assert(branch);
 
 				if (stat_tracking_info(branch, &num_ours,
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index c66bf79..24fc2ba 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -28,7 +28,10 @@ test_expect_success setup '
 	git update-ref refs/remotes/origin/master master &&
 	git remote add origin nowhere &&
 	git config branch.master.remote origin &&
-	git config branch.master.merge refs/heads/master
+	git config branch.master.merge refs/heads/master &&
+	git remote add myfork elsewhere &&
+	git config remote.pushdefault myfork &&
+	git config push.default current
 '
 
 test_atom() {
@@ -47,6 +50,7 @@ test_atom() {
 
 test_atom head refname refs/heads/master
 test_atom head upstream refs/remotes/origin/master
+test_atom head push refs/remotes/myfork/master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
@@ -83,6 +87,7 @@ test_atom head HEAD '*'
 
 test_atom tag refname refs/tags/testtag
 test_atom tag upstream ''
+test_atom tag push ''
 test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
@@ -347,6 +352,12 @@ test_expect_success 'Check that :track[short] works when upstream is invalid' '
 	test_cmp expected actual
 '
 
+test_expect_success '%(push) supports tracking specifiers, too' '
+	echo "[ahead 1]" >expected &&
+	git for-each-ref --format="%(push:track)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<EOF
 $(git rev-parse --short HEAD)
 EOF
-- 
2.4.0.rc3.477.gc25258d