{"thread":{"id":"39390","subject":"[PATCH v3 0/14] implement @{push} shorthand","startedAt":"2015-05-21T04:44:29Z","lastAt":"2015-05-22T00:49:11Z","messageCount":27,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":14},"messages":[{"id":"261734","messageId":"20150521044429.GA5857@peff.net","threadId":"39390","inReplyTo":null,"subject":"[PATCH v3 0/14] implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:44:29Z","receivedAt":"2015-05-21T04:44:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a re-roll of the series at:\n\n http://thread.gmane.org/gmane.comp.version-control.git/268185\n\nThe only changes here are the addition of patches 2 and 6, which are\nboth cleanups that help make the other patches more readable/sensible.\nThey are the same as what was posted during review of the thread linked\nabove. So there's nothing new here, but of course fresh reviews are\nwelcome.\n\n [01/14]: remote.c: drop default_remote_name variable\n [02/14]: remote.c: refactor setup of branch->merge list\n [03/14]: remote.c: drop \"remote\" pointer from \"struct branch\"\n [04/14]: remote.c: hoist branch.*.remote lookup out of remote_get_1\n [05/14]: remote.c: provide per-branch pushremote name\n [06/14]: remote.c: hoist read_config into remote_get_1\n [07/14]: remote.c: introduce branch_get_upstream helper\n [08/14]: remote.c: report specific errors from branch_get_upstream\n [09/14]: remote.c: add branch_get_push\n [10/14]: sha1_name: refactor upstream_mark\n [11/14]: sha1_name: refactor interpret_upstream_mark\n [12/14]: sha1_name: implement @{push} shorthand\n [13/14]: for-each-ref: use skip_prefix instead of starts_with\n [14/14]: for-each-ref: accept \"%(push)\" format\n\n-Peff\n"},{"id":"261735","messageId":"20150521044505.GA23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 01/14] remote.c: drop default_remote_name variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:05Z","receivedAt":"2015-05-21T04:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read the remote config from disk, we update a\ndefault_remote_name variable if we see branch.*.remote\nconfig for the current branch. This isn't wrong, or even all\nthat complicated, but it is a bit simpler (because it\nreduces our overall state) to just lazily compute the\ndefault when we need it.\n\nThe ulterior motive here is that the push config uses a\nsimilar structure, and _is_ much more complicated as a\nresult. That will be simplified in a future patch, and it's\nmore readable if the logic for remotes and push-remotes\nmatches.\n\nNote that we also used default_remote_name as a signal that\nthe remote config has been loaded; after this patch, we now\nuse an explicit flag.\n\nSigned-off-by: Jeff King \n---\n remote.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 68901b0..bec8b31 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -49,10 +49,8 @@ static int branches_alloc;\n static int branches_nr;\n \n static struct branch *current_branch;\n-static const char *default_remote_name;\n static const char *branch_pushremote_name;\n static const char *pushremote_name;\n-static int explicit_default_remote_name;\n \n static struct rewrites rewrites;\n static struct rewrites rewrites_push;\n@@ -367,12 +365,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\treturn 0;\n \t\tbranch = make_branch(name, subkey - name);\n \t\tif (!strcmp(subkey, \".remote\")) {\n-\t\t\tif (git_config_string(&branch->remote_name, key, value))\n-\t\t\t\treturn -1;\n-\t\t\tif (branch == current_branch) {\n-\t\t\t\tdefault_remote_name = branch->remote_name;\n-\t\t\t\texplicit_default_remote_name = 1;\n-\t\t\t}\n+\t\t\treturn git_config_string(&branch->remote_name, key, value);\n \t\t} else if (!strcmp(subkey, \".pushremote\")) {\n \t\t\tif (branch == current_branch)\n \t\t\t\tif (git_config_string(&branch_pushremote_name, key, value))\n@@ -501,12 +494,15 @@ static void alias_all_urls(void)\n \n static void read_config(void)\n {\n+\tstatic int loaded;\n \tunsigned char sha1[20];\n \tconst char *head_ref;\n \tint flag;\n-\tif (default_remote_name) /* did this already */\n+\n+\tif (loaded)\n \t\treturn;\n-\tdefault_remote_name = \"origin\";\n+\tloaded = 1;\n+\n \tcurrent_branch = NULL;\n \thead_ref = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n \tif (head_ref && (flag & REF_ISSYMREF) &&\n@@ -708,8 +704,11 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name\n \t\t\tname = pushremote_name;\n \t\t\tname_given = 1;\n \t\t} else {\n-\t\t\tname = default_remote_name;\n-\t\t\tname_given = explicit_default_remote_name;\n+\t\t\tif (current_branch && current_branch->remote_name) {\n+\t\t\t\tname = current_branch->remote_name;\n+\t\t\t\tname_given = 1;\n+\t\t\t} else\n+\t\t\t\tname = \"origin\";\n \t\t}\n \t}\n \n-- \n2.4.1.528.g00591e3\n"},{"id":"261736","messageId":"20150521044509.GB23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 02/14] remote.c: refactor setup of branch->merge list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:09Z","receivedAt":"2015-05-21T04:45:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call branch_get() to lookup or create a \"struct\nbranch\", we make sure the \"merge\" field is filled in so that\ncallers can access it. But the conditions under which we do\nso are a little confusing, and can lead to two funny\nsituations:\n\n 1. If there's no branch.*.remote config, we cannot provide\n branch->merge (because it is really just an application\n of branch.*.merge to our remote's refspecs). But\n branch->merge_nr may be non-zero, leading callers to be\n believe they can access branch->merge (e.g., in\n branch_merge_matches and elsewhere).\n\n It doesn't look like this can cause a segfault in\n practice, as most code paths dealing with merge config\n will bail early if there is no remote defined. But it's\n a bit of a dangerous construct.\n\n We can fix this by setting merge_nr to \"0\" explicitly\n when we realize that we have no merge config. Note that\n merge_nr also counts the \"merge_name\" fields (which we\n _do_ have; that's how merge_nr got incremented), so we\n will \"lose\" access to them, in the sense that we forget\n how many we had. But no callers actually care; we use\n merge_name only while iteratively reading the config,\n and then convert it to the final \"merge\" form the first\n time somebody calls branch_get().\n\n 2. We set up the \"merge\" field every time branch_get is\n called, even if it has already been done. This leaks\n memory.\n\n It's not a big deal in practice, since most code paths\n will access only one branch, or perhaps each branch\n only one time. But if you want to be pathological, you\n can leak arbitrary memory with:\n\n yes @{upstream} | head -1000 | git rev-list --stdin\n\n We can fix this by skipping setup when branch->merge is\n already non-NULL.\n\nIn addition to those two fixes, this patch pushes the \"do we\nneed to setup merge?\" logic down into set_merge, where it is\na bit easier to follow.\n\nSigned-off-by: Jeff King \n---\n remote.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex bec8b31..ac17e66 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret)\n \tunsigned char sha1[20];\n \tint i;\n \n+\tif (!ret)\n+\t\treturn; /* no branch */\n+\tif (ret->merge)\n+\t\treturn; /* already run */\n+\tif (!ret->remote_name || !ret->merge_nr) {\n+\t\t/*\n+\t\t * no merge config; let's make sure we don't confuse callers\n+\t\t * with a non-zero merge_nr but a NULL merge\n+\t\t */\n+\t\tret->merge_nr = 0;\n+\t\treturn;\n+\t}\n+\n \tret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));\n \tfor (i = 0; i < ret->merge_nr; i++) {\n \t\tret->merge[i] = xcalloc(1, sizeof(**ret->merge));\n@@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name)\n \t\tret = current_branch;\n \telse\n \t\tret = make_branch(name, 0);\n-\tif (ret && ret->remote_name) {\n+\tif (ret && ret->remote_name)\n \t\tret->remote = remote_get(ret->remote_name);\n-\t\tif (ret->merge_nr)\n-\t\t\tset_merge(ret);\n-\t}\n+\tset_merge(ret);\n \treturn ret;\n }\n \n-- \n2.4.1.528.g00591e3\n"},{"id":"261737","messageId":"20150521044512.GC23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 03/14] remote.c: drop \"remote\" pointer from \"struct branch\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:13Z","receivedAt":"2015-05-21T04:45:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we create each branch struct, we fill in the\n\"remote_name\" field from the config, and then fill in the\nactual \"remote\" field (with a \"struct remote\") based on that\nname. However, it turns out that nobody really cares about\nthe latter field. The only two sites that access it at all\nare:\n\n 1. git-merge, which uses it to notice when the branch does\n not have a remote defined. But we can easily replace this\n with looking at remote_name instead.\n\n 2. remote.c itself, when setting up the @{upstream} merge\n config. But we don't need to save the \"remote\" in the\n \"struct branch\" for that; we can just look it up for\n the duration of the operation.\n\nSo there is no need to have both fields; they are redundant\nwith each other (the struct remote contains the name, or you\ncan look up the struct from the name). It would be nice to\nsimplify this, especially as we are going to add matching\npushremote config in a future patch (and it would be nice to\nkeep them consistent).\n\nSo which one do we keep and which one do we get rid of?\n\nIf we had a lot of callers accessing the struct, it would be\nmore efficient to keep it (since you have to do a lookup to\ngo from the name to the struct, but not vice versa). But we\ndon't have a lot of callers; we have exactly one, so\nefficiency doesn't matter. We can decide this based on\nsimplicity and readability.\n\nAnd the meaning of the struct value is somewhat unclear. Is\nit always the remote matching remote_name? If remote_name is\nNULL (i.e., no per-branch config), does the struct fall back\nto the \"origin\" remote, or is it also NULL? These questions\nwill get even more tricky with pushremotes, whose fallback\nbehavior is more complicated. So let's just store the name,\nwhich pretty clearly represents the branch.*.remote config.\nAny lookup or fallback behavior can then be implemented in\nhelper functions.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-remote.txt | 4 ----\n builtin/merge.c | 2 +-\n remote.c | 7 ++++---\n remote.h | 1 -\n 4 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt\nindex 5d245aa..2cfdd22 100644\n--- a/Documentation/technical/api-remote.txt\n+++ b/Documentation/technical/api-remote.txt\n@@ -97,10 +97,6 @@ It contains:\n \n \tThe name of the remote listed in the configuration.\n \n-`remote`::\n-\n-\tThe struct remote for that remote.\n-\n `merge_name`::\n \n \tAn array of the \"merge\" lines in the configuration.\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f89f60e..85c54dc 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -933,7 +933,7 @@ static int setup_with_upstream(const char ***argv)\n \n \tif (!branch)\n \t\tdie(_(\"No current branch.\"));\n-\tif (!branch->remote)\n+\tif (!branch->remote_name)\n \t\tdie(_(\"No remote for the current branch.\"));\n \tif (!branch->merge_nr)\n \t\tdie(_(\"No default upstream defined for the current branch.\"));\ndiff --git a/remote.c b/remote.c\nindex ac17e66..c298a43 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1632,6 +1632,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \n static void set_merge(struct branch *ret)\n {\n+\tstruct remote *remote;\n \tchar *ref;\n \tunsigned char sha1[20];\n \tint i;\n@@ -1649,11 +1650,13 @@ static void set_merge(struct branch *ret)\n \t\treturn;\n \t}\n \n+\tremote = remote_get(ret->remote_name);\n+\n \tret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));\n \tfor (i = 0; i < ret->merge_nr; i++) {\n \t\tret->merge[i] = xcalloc(1, sizeof(**ret->merge));\n \t\tret->merge[i]->src = xstrdup(ret->merge_name[i]);\n-\t\tif (!remote_find_tracking(ret->remote, ret->merge[i]) ||\n+\t\tif (!remote_find_tracking(remote, ret->merge[i]) ||\n \t\t strcmp(ret->remote_name, \".\"))\n \t\t\tcontinue;\n \t\tif (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),\n@@ -1673,8 +1676,6 @@ struct branch *branch_get(const char *name)\n \t\tret = current_branch;\n \telse\n \t\tret = make_branch(name, 0);\n-\tif (ret && ret->remote_name)\n-\t\tret->remote = remote_get(ret->remote_name);\n \tset_merge(ret);\n \treturn ret;\n }\ndiff --git a/remote.h b/remote.h\nindex 02d66ce..4bb6672 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -203,7 +203,6 @@ struct branch {\n \tconst char *refname;\n \n \tconst char *remote_name;\n-\tstruct remote *remote;\n \n \tconst char **merge_name;\n \tstruct refspec **merge;\n-- \n2.4.1.528.g00591e3\n"},{"id":"261738","messageId":"20150521044516.GD23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 04/14] remote.c: hoist branch.*.remote lookup out of remote_get_1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:16Z","receivedAt":"2015-05-21T04:45:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'll want to use this logic as a fallback when looking up\nthe pushremote, so let's pull it out into its own function.\n\nWe don't technically need to make this available outside of\nremote.c, but doing so will provide a consistent API with\npushremote_for_branch, which we will add later.\n\nSigned-off-by: Jeff King \n---\n remote.c | 21 ++++++++++++++-------\n remote.h | 1 +\n 2 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c298a43..0d2976b 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -692,6 +692,18 @@ static int valid_remote_nick(const char *name)\n \treturn !strchr(name, '/'); /* no slash */\n }\n \n+const char *remote_for_branch(struct branch *branch, int *explicit)\n+{\n+\tif (branch && branch->remote_name) {\n+\t\tif (explicit)\n+\t\t\t*explicit = 1;\n+\t\treturn branch->remote_name;\n+\t}\n+\tif (explicit)\n+\t\t*explicit = 0;\n+\treturn \"origin\";\n+}\n+\n static struct remote *remote_get_1(const char *name, const char *pushremote_name)\n {\n \tstruct remote *ret;\n@@ -703,13 +715,8 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name\n \t\tif (pushremote_name) {\n \t\t\tname = pushremote_name;\n \t\t\tname_given = 1;\n-\t\t} else {\n-\t\t\tif (current_branch && current_branch->remote_name) {\n-\t\t\t\tname = current_branch->remote_name;\n-\t\t\t\tname_given = 1;\n-\t\t\t} else\n-\t\t\t\tname = \"origin\";\n-\t\t}\n+\t\t} else\n+\t\t\tname = remote_for_branch(current_branch, &name_given);\n \t}\n \n \tret = make_remote(name, 0);\ndiff --git a/remote.h b/remote.h\nindex 4bb6672..2a7e7a6 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -211,6 +211,7 @@ struct branch {\n };\n \n struct branch *branch_get(const char *name);\n+const char *remote_for_branch(struct branch *branch, int *explicit);\n \n int branch_has_merge_config(struct branch *branch);\n int branch_merge_matches(struct branch *, int n, const char *);\n-- \n2.4.1.528.g00591e3\n"},{"id":"261739","messageId":"20150521044519.GE23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 05/14] remote.c: provide per-branch pushremote name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:20Z","receivedAt":"2015-05-21T04:45:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When remote.c loads its config, it records the\nbranch.*.pushremote for the current branch along with the\nglobal remote.pushDefault value, and then binds them into a\nsingle value: the default push for the current branch. We\nthen pass this value (which may be NULL) to remote_get_1\nwhen looking up a remote for push.\n\nThis has a few downsides:\n\n 1. It's confusing. The early-binding of the \"current\n value\" led to bugs like the one fixed by 98b406f\n (remote: handle pushremote config in any order,\n 2014-02-24). And the fact that pushremotes fall back to\n ordinary remotes is not explicit at all; it happens\n because remote_get_1 cannot tell the difference between\n \"we are not asking for the push remote\" and \"there is\n no push remote configured\".\n\n 2. It throws away intermediate data. After read_config()\n finishes, we have no idea what the value of\n remote.pushDefault was, because the string has been\n overwritten by the current branch's\n branch.*.pushremote.\n\n 3. It doesn't record other data. We don't note the\n branch.*.pushremote value for anything but the current\n branch.\n\nLet's make this more like the fetch-remote config. We'll\nrecord the pushremote for each branch, and then explicitly\ncompute the correct remote for the current branch at the\ntime of reading.\n\nSigned-off-by: Jeff King \n---\n remote.c | 40 ++++++++++++++++++++++------------------\n remote.h | 2 ++\n 2 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0d2976b..a91d063 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -49,7 +49,6 @@ static int branches_alloc;\n static int branches_nr;\n \n static struct branch *current_branch;\n-static const char *branch_pushremote_name;\n static const char *pushremote_name;\n \n static struct rewrites rewrites;\n@@ -367,9 +366,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (!strcmp(subkey, \".remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n \t\t} else if (!strcmp(subkey, \".pushremote\")) {\n-\t\t\tif (branch == current_branch)\n-\t\t\t\tif (git_config_string(&branch_pushremote_name, key, value))\n-\t\t\t\t\treturn -1;\n+\t\t\treturn git_config_string(&branch->pushremote_name, key, value);\n \t\t} else if (!strcmp(subkey, \".merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n@@ -510,10 +507,6 @@ static void read_config(void)\n \t\tcurrent_branch = make_branch(head_ref, 0);\n \t}\n \tgit_config(handle_config, NULL);\n-\tif (branch_pushremote_name) {\n-\t\tfree((char *)pushremote_name);\n-\t\tpushremote_name = branch_pushremote_name;\n-\t}\n \talias_all_urls();\n }\n \n@@ -704,20 +697,31 @@ const char *remote_for_branch(struct branch *branch, int *explicit)\n \treturn \"origin\";\n }\n \n-static struct remote *remote_get_1(const char *name, const char *pushremote_name)\n+const char *pushremote_for_branch(struct branch *branch, int *explicit)\n+{\n+\tif (branch && branch->pushremote_name) {\n+\t\tif (explicit)\n+\t\t\t*explicit = 1;\n+\t\treturn branch->pushremote_name;\n+\t}\n+\tif (pushremote_name) {\n+\t\tif (explicit)\n+\t\t\t*explicit = 1;\n+\t\treturn pushremote_name;\n+\t}\n+\treturn remote_for_branch(branch, explicit);\n+}\n+\n+static struct remote *remote_get_1(const char *name,\n+\t\t\t\t const char *(*get_default)(struct branch *, int *))\n {\n \tstruct remote *ret;\n \tint name_given = 0;\n \n \tif (name)\n \t\tname_given = 1;\n-\telse {\n-\t\tif (pushremote_name) {\n-\t\t\tname = pushremote_name;\n-\t\t\tname_given = 1;\n-\t\t} else\n-\t\t\tname = remote_for_branch(current_branch, &name_given);\n-\t}\n+\telse\n+\t\tname = get_default(current_branch, &name_given);\n \n \tret = make_remote(name, 0);\n \tif (valid_remote_nick(name)) {\n@@ -738,13 +742,13 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name\n struct remote *remote_get(const char *name)\n {\n \tread_config();\n-\treturn remote_get_1(name, NULL);\n+\treturn remote_get_1(name, remote_for_branch);\n }\n \n struct remote *pushremote_get(const char *name)\n {\n \tread_config();\n-\treturn remote_get_1(name, pushremote_name);\n+\treturn remote_get_1(name, pushremote_for_branch);\n }\n \n int remote_is_configured(const char *name)\ndiff --git a/remote.h b/remote.h\nindex 2a7e7a6..30a11da 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -203,6 +203,7 @@ struct branch {\n \tconst char *refname;\n \n \tconst char *remote_name;\n+\tconst char *pushremote_name;\n \n \tconst char **merge_name;\n \tstruct refspec **merge;\n@@ -212,6 +213,7 @@ struct branch {\n \n struct branch *branch_get(const char *name);\n const char *remote_for_branch(struct branch *branch, int *explicit);\n+const char *pushremote_for_branch(struct branch *branch, int *explicit);\n \n int branch_has_merge_config(struct branch *branch);\n int branch_merge_matches(struct branch *, int n, const char *);\n-- \n2.4.1.528.g00591e3\n"},{"id":"261740","messageId":"20150521044523.GF23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 06/14] remote.c: hoist read_config into remote_get_1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:23Z","receivedAt":"2015-05-21T04:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before the previous commit, we had to make sure that\nread_config() was called before entering remote_get_1,\nbecause we needed to pass pushremote_name by value. But now\nthat we pass a function, we can let remote_get_1 handle\nloading the config itself, turning our wrappers into true\none-liners.\n\nSigned-off-by: Jeff King \n---\n remote.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex a91d063..e6b29b3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -718,6 +718,8 @@ static struct remote *remote_get_1(const char *name,\n \tstruct remote *ret;\n \tint name_given = 0;\n \n+\tread_config();\n+\n \tif (name)\n \t\tname_given = 1;\n \telse\n@@ -741,13 +743,11 @@ static struct remote *remote_get_1(const char *name,\n \n struct remote *remote_get(const char *name)\n {\n-\tread_config();\n \treturn remote_get_1(name, remote_for_branch);\n }\n \n struct remote *pushremote_get(const char *name)\n {\n-\tread_config();\n \treturn remote_get_1(name, pushremote_for_branch);\n }\n \n-- \n2.4.1.528.g00591e3\n"},{"id":"261741","messageId":"20150521044528.GG23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 07/14] remote.c: introduce branch_get_upstream helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:28Z","receivedAt":"2015-05-21T04:45:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the information needed to find the @{upstream} of a\nbranch is included in the branch struct, but callers have to\nnavigate a series of possible-NULL values to get there.\nLet's wrap that logic up in an easy-to-read helper.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 8 +++-----\n builtin/for-each-ref.c | 5 ++---\n builtin/log.c | 7 ++-----\n remote.c | 12 +++++++++---\n remote.h | 7 +++++++\n 5 files changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 258fe2f..1eb6215 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -123,14 +123,12 @@ static int branch_merged(int kind, const char *name,\n \n \tif (kind == REF_LOCAL_BRANCH) {\n \t\tstruct branch *branch = branch_get(name);\n+\t\tconst char *upstream = branch_get_upstream(branch);\n \t\tunsigned char sha1[20];\n \n-\t\tif (branch &&\n-\t\t branch->merge &&\n-\t\t branch->merge[0] &&\n-\t\t branch->merge[0]->dst &&\n+\t\tif (upstream &&\n \t\t (reference_name = reference_name_to_free =\n-\t\t resolve_refdup(branch->merge[0]->dst, RESOLVE_REF_READING,\n+\t\t resolve_refdup(upstream, RESOLVE_REF_READING,\n \t\t\t\t sha1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n \t}\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 83f9cf9..dc2a201 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -664,10 +664,9 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tcontinue;\n \t\t\tbranch = branch_get(ref->refname + 11);\n \n-\t\t\tif (!branch || !branch->merge || !branch->merge[0] ||\n-\t\t\t !branch->merge[0]->dst)\n+\t\t\trefname = branch_get_upstream(branch);\n+\t\t\tif (!refname)\n \t\t\t\tcontinue;\n-\t\t\trefname = branch->merge[0]->dst;\n \t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..fb61c08 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1632,16 +1632,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \tdefault:\n \t\tcurrent_branch = branch_get(NULL);\n-\t\tif (!current_branch || !current_branch->merge\n-\t\t\t\t\t|| !current_branch->merge[0]\n-\t\t\t\t\t|| !current_branch->merge[0]->dst) {\n+\t\tupstream = branch_get_upstream(current_branch);\n+\t\tif (!upstream) {\n \t\t\tfprintf(stderr, _(\"Could not find a tracked\"\n \t\t\t\t\t\" remote branch, please\"\n \t\t\t\t\t\" specify manually.\\n\"));\n \t\t\tusage_with_options(cherry_usage, options);\n \t\t}\n-\n-\t\tupstream = current_branch->merge[0]->dst;\n \t}\n \n \tinit_revisions(&revs, prefix);\ndiff --git a/remote.c b/remote.c\nindex e6b29b3..dca3442 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1705,6 +1705,13 @@ int branch_merge_matches(struct branch *branch,\n \treturn refname_match(branch->merge[i]->src, refname);\n }\n \n+const char *branch_get_upstream(struct branch *branch)\n+{\n+\tif (!branch || !branch->merge || !branch->merge[0])\n+\t\treturn NULL;\n+\treturn branch->merge[0]->dst;\n+}\n+\n static int ignore_symref_update(const char *refname)\n {\n \tunsigned char sha1[20];\n@@ -1914,12 +1921,11 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \tint rev_argc;\n \n \t/* Cannot stat unless we are marked to build on top of somebody else. */\n-\tif (!branch ||\n-\t !branch->merge || !branch->merge[0] || !branch->merge[0]->dst)\n+\tbase = branch_get_upstream(branch);\n+\tif (!base)\n \t\treturn 0;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n-\tbase = branch->merge[0]->dst;\n \tif (read_ref(base, sha1))\n \t\treturn -1;\n \ttheirs = lookup_commit_reference(sha1);\ndiff --git a/remote.h b/remote.h\nindex 30a11da..d968952 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -218,6 +218,13 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit);\n int branch_has_merge_config(struct branch *branch);\n int branch_merge_matches(struct branch *, int n, const char *);\n \n+/**\n+ * Return the fully-qualified refname of the tracking branch for `branch`.\n+ * I.e., what \"branch@{upstream}\" would give you. Returns NULL if no\n+ * upstream is defined.\n+ */\n+const char *branch_get_upstream(struct branch *branch);\n+\n /* Flags to match_refs. */\n enum match_refs_flags {\n \tMATCH_REFS_NONE\t\t= 0,\n-- \n2.4.1.528.g00591e3\n"},{"id":"261742","messageId":"20150521044532.GH23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:32Z","receivedAt":"2015-05-21T04:45:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the previous commit introduced the branch_get_upstream\nhelper, there was one call-site that could not be converted:\nthe one in sha1_name.c, which gives detailed error messages\nfor each possible failure.\n\nLet's teach the helper to optionally report these specific\nerrors. This lets us convert another callsite, and means we\ncan use the helper in other locations that want to give the\nsame error messages.\n\nThe logic and error messages come straight from sha1_name.c,\nwith the exception that we start each error with a lowercase\nletter, as is our usual style (note that a few tests need\nupdated as a result).\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 2 +-\n builtin/for-each-ref.c | 2 +-\n builtin/log.c | 2 +-\n remote.c | 33 +++++++++++++++++++++++++++++----\n remote.h | 6 +++++-\n sha1_name.c | 25 +++++++------------------\n t/t1507-rev-parse-upstream.sh | 8 ++++----\n 7 files changed, 48 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 1eb6215..cc55ff2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -123,7 +123,7 @@ static int branch_merged(int kind, const char *name,\n \n \tif (kind == REF_LOCAL_BRANCH) {\n \t\tstruct branch *branch = branch_get(name);\n-\t\tconst char *upstream = branch_get_upstream(branch);\n+\t\tconst char *upstream = branch_get_upstream(branch, NULL);\n \t\tunsigned char sha1[20];\n \n \t\tif (upstream &&\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex dc2a201..18d209b 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -664,7 +664,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tcontinue;\n \t\t\tbranch = branch_get(ref->refname + 11);\n \n-\t\t\trefname = branch_get_upstream(branch);\n+\t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fb61c08..6faeb82 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1632,7 +1632,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \tdefault:\n \t\tcurrent_branch = branch_get(NULL);\n-\t\tupstream = branch_get_upstream(current_branch);\n+\t\tupstream = branch_get_upstream(current_branch, NULL);\n \t\tif (!upstream) {\n \t\t\tfprintf(stderr, _(\"Could not find a tracked\"\n \t\t\t\t\t\" remote branch, please\"\ndiff --git a/remote.c b/remote.c\nindex dca3442..1b7051a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1705,10 +1705,35 @@ int branch_merge_matches(struct branch *branch,\n \treturn refname_match(branch->merge[i]->src, refname);\n }\n \n-const char *branch_get_upstream(struct branch *branch)\n+__attribute((format (printf,2,3)))\n+static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n {\n-\tif (!branch || !branch->merge || !branch->merge[0])\n-\t\treturn NULL;\n+\tif (err) {\n+\t\tva_list ap;\n+\t\tva_start(ap, fmt);\n+\t\tstrbuf_vaddf(err, fmt, ap);\n+\t\tva_end(ap);\n+\t}\n+\treturn NULL;\n+}\n+\n+const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n+{\n+\tif (!branch)\n+\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n+\tif (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {\n+\t\tif (!ref_exists(branch->refname))\n+\t\t\treturn error_buf(err, _(\"no such branch: '%s'\"),\n+\t\t\t\t\t branch->name);\n+\t\tif (!branch->merge)\n+\t\t\treturn error_buf(err,\n+\t\t\t\t\t _(\"no upstream configured for branch '%s'\"),\n+\t\t\t\t\t branch->name);\n+\t\treturn error_buf(err,\n+\t\t\t\t _(\"upstream branch '%s' not stored as a remote-tracking branch\"),\n+\t\t\t\t branch->merge[0]->src);\n+\t}\n+\n \treturn branch->merge[0]->dst;\n }\n \n@@ -1921,7 +1946,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \tint rev_argc;\n \n \t/* Cannot stat unless we are marked to build on top of somebody else. */\n-\tbase = branch_get_upstream(branch);\n+\tbase = branch_get_upstream(branch, NULL);\n \tif (!base)\n \t\treturn 0;\n \ndiff --git a/remote.h b/remote.h\nindex d968952..03ca005 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -222,8 +222,12 @@ int branch_merge_matches(struct branch *, int n, const char *);\n * Return the fully-qualified refname of the tracking branch for `branch`.\n * I.e., what \"branch@{upstream}\" would give you. Returns NULL if no\n * upstream is defined.\n+ *\n+ * If `err` is not NULL and no upstream is defined, a more specific error\n+ * message is recorded there (if the function does not return NULL, then\n+ * `err` is not touched).\n */\n-const char *branch_get_upstream(struct branch *branch);\n+const char *branch_get_upstream(struct branch *branch, struct strbuf *err);\n \n /* Flags to match_refs. */\n enum match_refs_flags {\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6d10f05..461157a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1059,27 +1059,16 @@ static const char *get_upstream_branch(const char *branch_buf, int len)\n {\n \tchar *branch = xstrndup(branch_buf, len);\n \tstruct branch *upstream = branch_get(*branch ? branch : NULL);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tconst char *ret;\n \n-\t/*\n-\t * Upstream can be NULL only if branch refers to HEAD and HEAD\n-\t * points to something different than a branch.\n-\t */\n-\tif (!upstream)\n-\t\tdie(_(\"HEAD does not point to a branch\"));\n-\tif (!upstream->merge || !upstream->merge[0]->dst) {\n-\t\tif (!ref_exists(upstream->refname))\n-\t\t\tdie(_(\"No such branch: '%s'\"), branch);\n-\t\tif (!upstream->merge) {\n-\t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n-\t\t\t\tupstream->name);\n-\t\t}\n-\t\tdie(\n-\t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n-\t\t\tupstream->merge[0]->src);\n-\t}\n \tfree(branch);\n \n-\treturn upstream->merge[0]->dst;\n+\tret = branch_get_upstream(upstream, &err);\n+\tif (!ret)\n+\t\tdie(\"%s\", err.buf);\n+\n+\treturn ret;\n }\n \n static int interpret_upstream_mark(const char *name, int namelen,\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 1978947..46ef1f2 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -150,7 +150,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '\n \n test_expect_success 'branch@{u} error message when no upstream' '\n \tcat >expect <<-EOF &&\n-\tfatal: No upstream configured for branch ${sq}non-tracking${sq}\n+\tfatal: no upstream configured for branch ${sq}non-tracking${sq}\n \tEOF\n \terror_message non-tracking@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -158,7 +158,7 @@ test_expect_success 'branch@{u} error message when no upstream' '\n \n test_expect_success '@{u} error message when no upstream' '\n \tcat >expect <<-EOF &&\n-\tfatal: No upstream configured for branch ${sq}master${sq}\n+\tfatal: no upstream configured for branch ${sq}master${sq}\n \tEOF\n \ttest_must_fail git rev-parse --verify @{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -166,7 +166,7 @@ test_expect_success '@{u} error message when no upstream' '\n \n test_expect_success 'branch@{u} error message with misspelt branch' '\n \tcat >expect <<-EOF &&\n-\tfatal: No such branch: ${sq}no-such-branch${sq}\n+\tfatal: no such branch: ${sq}no-such-branch${sq}\n \tEOF\n \terror_message no-such-branch@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -183,7 +183,7 @@ test_expect_success '@{u} error message when not on a branch' '\n \n test_expect_success 'branch@{u} error message if upstream branch not fetched' '\n \tcat >expect <<-EOF &&\n-\tfatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch\n+\tfatal: upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch\n \tEOF\n \terror_message bad-upstream@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n-- \n2.4.1.528.g00591e3\n"},{"id":"261743","messageId":"20150521044536.GI23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 09/14] remote.c: add branch_get_push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:36Z","receivedAt":"2015-05-21T04:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a triangular workflow, the place you pull from and the\nplace you push to may be different. As we have\nbranch_get_upstream for the former, this patch adds\nbranch_get_push for the latter (and as the former implements\n@{upstream}, so will this implement @{push} in a future\npatch).\n\nNote that the memory-handling for the return value bears\nsome explanation. Some code paths require allocating a new\nstring, and some let us return an existing string. We should\nprovide a consistent interface to the caller, so it knows\nwhether to free the result or not.\n\nWe could do so by xstrdup-ing any existing strings, and\nhaving the caller always free. But that makes us\ninconsistent with branch_get_upstream, so we would prefer to\nsimply take ownership of the resulting string. We do so by\nstoring it inside the \"struct branch\", just as we do with\nthe upstream refname (in that case we compute it when the\nbranch is created, but there's no reason not to just fill\nit in lazily in this case).\n\nSigned-off-by: Jeff King \n---\n remote.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n remote.h | 10 ++++++++\n 2 files changed, 95 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 1b7051a..be45a39 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1737,6 +1737,91 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n \treturn branch->merge[0]->dst;\n }\n \n+static const char *tracking_for_push_dest(struct remote *remote,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t struct strbuf *err)\n+{\n+\tchar *ret;\n+\n+\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n+\tif (!ret)\n+\t\treturn error_buf(err,\n+\t\t\t\t _(\"push destination '%s' on remote '%s' has no local tracking branch\"),\n+\t\t\t\t refname, remote->name);\n+\treturn ret;\n+}\n+\n+static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n+{\n+\tstruct remote *remote;\n+\n+\tif (!branch)\n+\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n+\n+\tremote = remote_get(pushremote_for_branch(branch, NULL));\n+\tif (!remote)\n+\t\treturn error_buf(err,\n+\t\t\t\t _(\"branch '%s' has no remote for pushing\"),\n+\t\t\t\t branch->name);\n+\n+\tif (remote->push_refspec_nr) {\n+\t\tchar *dst;\n+\t\tconst char *ret;\n+\n+\t\tdst = apply_refspecs(remote->push, remote->push_refspec_nr,\n+\t\t\t\t branch->refname);\n+\t\tif (!dst)\n+\t\t\treturn error_buf(err,\n+\t\t\t\t\t _(\"push refspecs for '%s' do not include '%s'\"),\n+\t\t\t\t\t remote->name, branch->name);\n+\n+\t\tret = tracking_for_push_dest(remote, dst, err);\n+\t\tfree(dst);\n+\t\treturn ret;\n+\t}\n+\n+\tif (remote->mirror)\n+\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\n+\tswitch (push_default) {\n+\tcase PUSH_DEFAULT_NOTHING:\n+\t\treturn error_buf(err, _(\"push has no destination (push.default is 'nothing')\"));\n+\n+\tcase PUSH_DEFAULT_MATCHING:\n+\tcase PUSH_DEFAULT_CURRENT:\n+\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\n+\tcase PUSH_DEFAULT_UPSTREAM:\n+\t\treturn branch_get_upstream(branch, err);\n+\n+\tcase PUSH_DEFAULT_UNSPECIFIED:\n+\tcase PUSH_DEFAULT_SIMPLE:\n+\t\t{\n+\t\t\tconst char *up, *cur;\n+\n+\t\t\tup = branch_get_upstream(branch, err);\n+\t\t\tif (!up)\n+\t\t\t\treturn NULL;\n+\t\t\tcur = tracking_for_push_dest(remote, branch->refname, err);\n+\t\t\tif (!cur)\n+\t\t\t\treturn NULL;\n+\t\t\tif (strcmp(cur, up))\n+\t\t\t\treturn error_buf(err,\n+\t\t\t\t\t\t _(\"cannot resolve 'simple' push to a single destination\"));\n+\t\t\treturn cur;\n+\t\t}\n+\t}\n+\n+\tdie(\"BUG: unhandled push situation\");\n+}\n+\n+const char *branch_get_push(struct branch *branch, struct strbuf *err)\n+{\n+\tif (!branch->push_tracking_ref)\n+\t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n+\treturn branch->push_tracking_ref;\n+}\n+\n static int ignore_symref_update(const char *refname)\n {\n \tunsigned char sha1[20];\ndiff --git a/remote.h b/remote.h\nindex 03ca005..3326f2b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -209,6 +209,8 @@ struct branch {\n \tstruct refspec **merge;\n \tint merge_nr;\n \tint merge_alloc;\n+\n+\tconst char *push_tracking_ref;\n };\n \n struct branch *branch_get(const char *name);\n@@ -229,6 +231,14 @@ int branch_merge_matches(struct branch *, int n, const char *);\n */\n const char *branch_get_upstream(struct branch *branch, struct strbuf *err);\n \n+/**\n+ * Return the tracking branch that corresponds to the ref we would push to\n+ * given a bare `git push` while `branch` is checked out.\n+ *\n+ * The return value and `err` conventions match those of `branch_get_upstream`.\n+ */\n+const char *branch_get_push(struct branch *branch, struct strbuf *err);\n+\n /* Flags to match_refs. */\n enum match_refs_flags {\n \tMATCH_REFS_NONE\t\t= 0,\n-- \n2.4.1.528.g00591e3\n"},{"id":"261745","messageId":"20150521044539.GJ23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 10/14] sha1_name: refactor upstream_mark","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:39Z","receivedAt":"2015-05-21T04:45:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We will be adding new mark types in the future, so separate\nthe suffix data from the logic.\n\nSigned-off-by: Jeff King \n---\n sha1_name.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 461157a..1005f45 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -415,12 +415,12 @@ static int ambiguous_path(const char *path, int len)\n \treturn slash;\n }\n \n-static inline int upstream_mark(const char *string, int len)\n+static inline int at_mark(const char *string, int len,\n+\t\t\t const char **suffix, int nr)\n {\n-\tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n \tint i;\n \n-\tfor (i = 0; i < ARRAY_SIZE(suffix); i++) {\n+\tfor (i = 0; i < nr; i++) {\n \t\tint suffix_len = strlen(suffix[i]);\n \t\tif (suffix_len <= len\n \t\t && !memcmp(string, suffix[i], suffix_len))\n@@ -429,6 +429,12 @@ static inline int upstream_mark(const char *string, int len)\n \treturn 0;\n }\n \n+static inline int upstream_mark(const char *string, int len)\n+{\n+\tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n+\treturn at_mark(string, len, suffix, ARRAY_SIZE(suffix));\n+}\n+\n static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);\n \n-- \n2.4.1.528.g00591e3\n"},{"id":"261744","messageId":"20150521044543.GK23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 11/14] sha1_name: refactor interpret_upstream_mark","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:43Z","receivedAt":"2015-05-21T04:45:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that most of the logic for our local get_upstream_branch\nhas been pushed into the generic branch_get_upstream, we can\nfold the remainder into interpret_upstream_mark.\n\nFurthermore, what remains is generic to any branch-related\n\"@{foo}\" we might add in the future, and there's enough\nboilerplate that we'd like to reuse it. Let's parameterize\nthe two operations (parsing the mark and computing its\nvalue) so that we can reuse this for \"@{push}\" in the near\nfuture.\n\nSigned-off-by: Jeff King \n---\n sha1_name.c | 44 +++++++++++++++++++++++---------------------\n 1 file changed, 23 insertions(+), 21 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 1005f45..506e0c9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1061,35 +1061,36 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)\n \tfree(s);\n }\n \n-static const char *get_upstream_branch(const char *branch_buf, int len)\n-{\n-\tchar *branch = xstrndup(branch_buf, len);\n-\tstruct branch *upstream = branch_get(*branch ? branch : NULL);\n-\tstruct strbuf err = STRBUF_INIT;\n-\tconst char *ret;\n-\n-\tfree(branch);\n-\n-\tret = branch_get_upstream(upstream, &err);\n-\tif (!ret)\n-\t\tdie(\"%s\", err.buf);\n-\n-\treturn ret;\n-}\n-\n-static int interpret_upstream_mark(const char *name, int namelen,\n-\t\t\t\t int at, struct strbuf *buf)\n+static int interpret_branch_mark(const char *name, int namelen,\n+\t\t\t\t int at, struct strbuf *buf,\n+\t\t\t\t int (*get_mark)(const char *, int),\n+\t\t\t\t const char *(*get_data)(struct branch *,\n+\t\t\t\t\t\t\t struct strbuf *))\n {\n \tint len;\n+\tstruct branch *branch;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tconst char *value;\n \n-\tlen = upstream_mark(name + at, namelen - at);\n+\tlen = get_mark(name + at, namelen - at);\n \tif (!len)\n \t\treturn -1;\n \n \tif (memchr(name, ':', at))\n \t\treturn -1;\n \n-\tset_shortened_ref(buf, get_upstream_branch(name, at));\n+\tif (at) {\n+\t\tchar *name_str = xmemdupz(name, at);\n+\t\tbranch = branch_get(name_str);\n+\t\tfree(name_str);\n+\t} else\n+\t\tbranch = branch_get(NULL);\n+\n+\tvalue = get_data(branch, &err);\n+\tif (!value)\n+\t\tdie(\"%s\", err.buf);\n+\n+\tset_shortened_ref(buf, value);\n \treturn len + at;\n }\n \n@@ -1140,7 +1141,8 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\tif (len > 0)\n \t\t\treturn reinterpret(name, namelen, len, buf);\n \n-\t\tlen = interpret_upstream_mark(name, namelen, at - name, buf);\n+\t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n+\t\t\t\t\t upstream_mark, branch_get_upstream);\n \t\tif (len > 0)\n \t\t\treturn len;\n \t}\n-- \n2.4.1.528.g00591e3\n"},{"id":"261748","messageId":"20150521044547.GL23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 12/14] sha1_name: implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:47Z","receivedAt":"2015-05-21T04:45:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a triangular workflow, each branch may have two distinct\npoints of interest: the @{upstream} that you normally pull\nfrom, and the destination that you normally push to. There\nisn't a shorthand for the latter, but it's useful to have.\n\nFor instance, you may want to know which commits you haven't\npushed yet:\n\n git log @{push}..\n\nOr as a more complicated example, imagine that you normally\npull changes from origin/master (which you set as your\n@{upstream}), and push changes to your own personal fork\n(e.g., as myfork/topic). You may push to your fork from\nmultiple machines, requiring you to integrate the changes\nfrom the push destination, rather than upstream. With this\npatch, you can just do:\n\n git rebase @{push}\n\nrather than typing out the full name.\n\nThe heavy lifting is all done by branch_get_push; here we\njust wire it up to the \"@{push}\" syntax.\n\nSigned-off-by: Jeff King \n---\n Documentation/revisions.txt | 25 ++++++++++++++++++\n sha1_name.c | 14 +++++++++-\n t/t1514-rev-parse-push.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 101 insertions(+), 1 deletion(-)\n create mode 100755 t/t1514-rev-parse-push.sh\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex 0796118..d85e303 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -98,6 +98,31 @@ some output processing may assume ref names in UTF-8.\n `branch..merge`). A missing branchname defaults to the\n current one.\n \n+'@\\{push\\}', e.g. 'master@\\{push\\}', '@\\{push\\}'::\n+ The suffix '@\\{push}' reports the branch \"where we would push to\" if\n+ `git push` were run while `branchname` was checked out (or the current\n+ 'HEAD' if no branchname is specified). Since our push destination is\n+ in a remote repository, of course, we report the local tracking branch\n+ that corresponds to that branch (i.e., something in 'refs/remotes/').\n++\n+Here's an example to make it more clear:\n++\n+------------------------------\n+$ git config push.default current\n+$ git config remote.pushdefault myfork\n+$ git checkout -b mybranch origin/master\n+\n+$ git rev-parse --symbolic-full-name @{upstream}\n+refs/remotes/origin/master\n+\n+$ git rev-parse --symbolic-full-name @{push}\n+refs/remotes/myfork/mybranch\n+------------------------------\n++\n+Note in the example that we set up a triangular workflow, where we pull\n+from one location and push to another. In a non-triangular workflow,\n+'@\\{push}' is the same as '@\\{upstream}', and there is no need for it.\n+\n '{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::\n A suffix '{caret}' to a revision parameter means the first parent of\n that commit object. '{caret}' means the th parent (i.e.\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 506e0c9..1096943 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -435,6 +435,12 @@ static inline int upstream_mark(const char *string, int len)\n \treturn at_mark(string, len, suffix, ARRAY_SIZE(suffix));\n }\n \n+static inline int push_mark(const char *string, int len)\n+{\n+\tconst char *suffix[] = { \"@{push}\" };\n+\treturn at_mark(string, len, suffix, ARRAY_SIZE(suffix));\n+}\n+\n static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);\n \n@@ -482,7 +488,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n \t\t\t\t\tnth_prior = 1;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n-\t\t\t\tif (!upstream_mark(str + at, len - at)) {\n+\t\t\t\tif (!upstream_mark(str + at, len - at) &&\n+\t\t\t\t !push_mark(str + at, len - at)) {\n \t\t\t\t\treflog_len = (len-1) - (at+2);\n \t\t\t\t\tlen = at;\n \t\t\t\t}\n@@ -1145,6 +1152,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\t\t\t\t upstream_mark, branch_get_upstream);\n \t\tif (len > 0)\n \t\t\treturn len;\n+\n+\t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n+\t\t\t\t\t push_mark, branch_get_push);\n+\t\tif (len > 0)\n+\t\t\treturn len;\n \t}\n \n \treturn -1;\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nnew file mode 100755\nindex 0000000..7214f5b\n--- /dev/null\n+++ b/t/t1514-rev-parse-push.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+\n+test_description='test @{push} syntax'\n+. ./test-lib.sh\n+\n+resolve () {\n+\techo \"$2\" >expect &&\n+\tgit rev-parse --symbolic-full-name \"$1\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'setup' '\n+\tgit init --bare parent.git &&\n+\tgit init --bare other.git &&\n+\tgit remote add origin parent.git &&\n+\tgit remote add other other.git &&\n+\ttest_commit base &&\n+\tgit push origin HEAD &&\n+\tgit branch --set-upstream-to=origin/master master &&\n+\tgit branch --track topic origin/master &&\n+\tgit push origin topic &&\n+\tgit push other topic\n+'\n+\n+test_expect_success '@{push} with default=nothing' '\n+\ttest_config push.default nothing &&\n+\ttest_must_fail git rev-parse master@{push}\n+'\n+\n+test_expect_success '@{push} with default=simple' '\n+\ttest_config push.default simple &&\n+\tresolve master@{push} refs/remotes/origin/master\n+'\n+\n+test_expect_success 'triangular @{push} fails with default=simple' '\n+\ttest_config push.default simple &&\n+\ttest_must_fail git rev-parse topic@{push}\n+'\n+\n+test_expect_success '@{push} with default=current' '\n+\ttest_config push.default current &&\n+\tresolve topic@{push} refs/remotes/origin/topic\n+'\n+\n+test_expect_success '@{push} with default=matching' '\n+\ttest_config push.default matching &&\n+\tresolve topic@{push} refs/remotes/origin/topic\n+'\n+\n+test_expect_success '@{push} with pushremote defined' '\n+\ttest_config push.default current &&\n+\ttest_config branch.topic.pushremote other &&\n+\tresolve topic@{push} refs/remotes/other/topic\n+'\n+\n+test_expect_success '@{push} with push refspecs' '\n+\ttest_config push.default nothing &&\n+\ttest_config remote.origin.push refs/heads/*:refs/heads/magic/* &&\n+\tgit push &&\n+\tresolve topic@{push} refs/remotes/origin/magic/topic\n+'\n+\n+test_done\n-- \n2.4.1.528.g00591e3\n"},{"id":"261746","messageId":"20150521044551.GM23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 13/14] for-each-ref: use skip_prefix instead of starts_with","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:51Z","receivedAt":"2015-05-21T04:45:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us having to maintain a magic number to skip past\nthe matched prefix.\n\nSigned-off-by: Jeff King \n---\n builtin/for-each-ref.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 18d209b..345d8dd 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -659,10 +659,12 @@ static void populate_value(struct refinfo *ref)\n \t\telse if (starts_with(name, \"symref\"))\n \t\t\trefname = ref->symref ? ref->symref : \"\";\n \t\telse if (starts_with(name, \"upstream\")) {\n+\t\t\tconst char *branch_name;\n \t\t\t/* only local branches may have an upstream */\n-\t\t\tif (!starts_with(ref->refname, \"refs/heads/\"))\n+\t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n+\t\t\t\t\t &branch_name))\n \t\t\t\tcontinue;\n-\t\t\tbranch = branch_get(ref->refname + 11);\n+\t\t\tbranch = branch_get(branch_name);\n \n \t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (!refname)\n-- \n2.4.1.528.g00591e3\n"},{"id":"261747","messageId":"20150521044555.GN23409@peff.net","threadId":"39390","inReplyTo":"20150521044429.GA5857@peff.net","subject":"[PATCH v3 14/14] for-each-ref: accept \"%(push)\" format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T04:45:55Z","receivedAt":"2015-05-21T04:45:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Just as we have \"%(upstream)\" to report the \"@{upstream}\"\nfor each ref, this patch adds \"%(push)\" to match \"@{push}\".\nIt supports the same tracking format modifiers as upstream\n(because you may want to know, for example, which branches\nhave commits to push).\n\nSigned-off-by: Jeff King \n---\n Documentation/git-for-each-ref.txt | 6 ++++++\n builtin/for-each-ref.c | 17 +++++++++++++++--\n t/t6300-for-each-ref.sh | 13 ++++++++++++-\n 3 files changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 4240875..7f8d9a5 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -97,6 +97,12 @@ upstream::\n \tor \"=\" (in sync). Has no effect if the ref does not have\n \ttracking information associated with it.\n \n+push::\n+\tThe name of a local ref which represents the `@{push}` location\n+\tfor the displayed ref. Respects `:short`, `:track`, and\n+\t`:trackshort` options as `upstream` does. Produces an empty\n+\tstring if no `@{push}` ref is configured.\n+\n HEAD::\n \t'*' if HEAD matches current ref (the checked out branch), ' '\n \totherwise.\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 345d8dd..6847400 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -74,6 +74,7 @@ static struct {\n \t{ \"contents:body\" },\n \t{ \"contents:signature\" },\n \t{ \"upstream\" },\n+\t{ \"push\" },\n \t{ \"symref\" },\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n@@ -669,6 +670,16 @@ static void populate_value(struct refinfo *ref)\n \t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"push\")) {\n+\t\t\tconst char *branch_name;\n+\t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n+\t\t\t\t\t &branch_name))\n+\t\t\t\tcontinue;\n+\t\t\tbranch = branch_get(branch_name);\n+\n+\t\t\trefname = branch_get_push(branch, NULL);\n+\t\t\tif (!refname)\n+\t\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n@@ -714,7 +725,8 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n \t\t\telse if (!strcmp(formatp, \"track\") &&\n-\t\t\t\t starts_with(name, \"upstream\")) {\n+\t\t\t\t (starts_with(name, \"upstream\") ||\n+\t\t\t\t starts_with(name, \"push\"))) {\n \t\t\t\tchar buf[40];\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n@@ -736,7 +748,8 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n-\t\t\t\t starts_with(name, \"upstream\")) {\n+\t\t\t\t (starts_with(name, \"upstream\") ||\n+\t\t\t\t starts_with(name, \"push\"))) {\n \t\t\t\tassert(branch);\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c66bf79..24fc2ba 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -28,7 +28,10 @@ test_expect_success setup '\n \tgit update-ref refs/remotes/origin/master master &&\n \tgit remote add origin nowhere &&\n \tgit config branch.master.remote origin &&\n-\tgit config branch.master.merge refs/heads/master\n+\tgit config branch.master.merge refs/heads/master &&\n+\tgit remote add myfork elsewhere &&\n+\tgit config remote.pushdefault myfork &&\n+\tgit config push.default current\n '\n \n test_atom() {\n@@ -47,6 +50,7 @@ test_atom() {\n \n test_atom head refname refs/heads/master\n test_atom head upstream refs/remotes/origin/master\n+test_atom head push refs/remotes/myfork/master\n test_atom head objecttype commit\n test_atom head objectsize 171\n test_atom head objectname $(git rev-parse refs/heads/master)\n@@ -83,6 +87,7 @@ test_atom head HEAD '*'\n \n test_atom tag refname refs/tags/testtag\n test_atom tag upstream ''\n+test_atom tag push ''\n test_atom tag objecttype tag\n test_atom tag objectsize 154\n test_atom tag objectname $(git rev-parse refs/tags/testtag)\n@@ -347,6 +352,12 @@ test_expect_success 'Check that :track[short] works when upstream is invalid' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '%(push) supports tracking specifiers, too' '\n+\techo \"[ahead 1]\" >expected &&\n+\tgit for-each-ref --format=\"%(push:track)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected < This is a re-roll of the series at:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/268185\n> \n> The only changes here are the addition of patches 2 and 6, which are\n> both cleanups that help make the other patches more readable/sensible.\n> They are the same as what was posted during review of the thread linked\n> above. So there's nothing new here, but of course fresh reviews are\n> welcome.\n\nActually, that's not quite true; I forgot to mention one change:\n\n> [03/14]: remote.c: drop \"remote\" pointer from \"struct branch\"\n\nIn this version, this one remembers to also update the API\ndocumentation.\n\n-Peff\n"},{"id":"261820","messageId":"xmqqfv6pkek8.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521044509.GB23409@peff.net","subject":"Re: [PATCH v3 02/14] remote.c: refactor setup of branch->merge list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T17:47:51Z","receivedAt":"2015-05-21T17:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we call branch_get() to lookup or create a \"struct\n> branch\", we make sure the \"merge\" field is filled in so that\n> callers can access it. But the conditions under which we do\n> so are a little confusing, and can lead to two funny\n> situations:\n> ...\n> In addition to those two fixes, this patch pushes the \"do we\n> need to setup merge?\" logic down into set_merge, where it is\n> a bit easier to follow.\n\nNicely done. Thanks.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> remote.c | 19 +++++++++++++++----\n> 1 file changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index bec8b31..ac17e66 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret)\n> \tunsigned char sha1[20];\n> \tint i;\n> \n> +\tif (!ret)\n> +\t\treturn; /* no branch */\n> +\tif (ret->merge)\n> +\t\treturn; /* already run */\n> +\tif (!ret->remote_name || !ret->merge_nr) {\n> +\t\t/*\n> +\t\t * no merge config; let's make sure we don't confuse callers\n> +\t\t * with a non-zero merge_nr but a NULL merge\n> +\t\t */\n> +\t\tret->merge_nr = 0;\n> +\t\treturn;\n> +\t}\n> +\n> \tret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));\n> \tfor (i = 0; i < ret->merge_nr; i++) {\n> \t\tret->merge[i] = xcalloc(1, sizeof(**ret->merge));\n> @@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name)\n> \t\tret = current_branch;\n> \telse\n> \t\tret = make_branch(name, 0);\n> -\tif (ret && ret->remote_name) {\n> +\tif (ret && ret->remote_name)\n> \t\tret->remote = remote_get(ret->remote_name);\n> -\t\tif (ret->merge_nr)\n> -\t\t\tset_merge(ret);\n> -\t}\n> +\tset_merge(ret);\n> \treturn ret;\n> }\n"},{"id":"261824","messageId":"xmqqbnhdkdne.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521044528.GG23409@peff.net","subject":"Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T18:07:33Z","receivedAt":"2015-05-21T18:07:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> All of the information needed to find the @{upstream} of a\n> branch is included in the branch struct, but callers have to\n> navigate a series of possible-NULL values to get there.\n> Let's wrap that logic up in an easy-to-read helper.\n>\n> Signed-off-by: Jeff King \n\nThis step in the whole series is a gem. I cannot believe that we\nwere content having to repeat that \"branch->merge[0]->dst if we can\ndereference down to that level\" this many times. Nice clean-up.\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 258fe2f..1eb6215 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -123,14 +123,12 @@ static int branch_merged(int kind, const char *name,\n> \n> \tif (kind == REF_LOCAL_BRANCH) {\n> \t\tstruct branch *branch = branch_get(name);\n> +\t\tconst char *upstream = branch_get_upstream(branch);\n> \t\tunsigned char sha1[20];\n> \n> -\t\tif (branch &&\n> -\t\t branch->merge &&\n> -\t\t branch->merge[0] &&\n> -\t\t branch->merge[0]->dst &&\n> +\t\tif (upstream &&\n> \t\t (reference_name = reference_name_to_free =\n> -\t\t resolve_refdup(branch->merge[0]->dst, RESOLVE_REF_READING,\n> +\t\t resolve_refdup(upstream, RESOLVE_REF_READING,\n> \t\t\t\t sha1, NULL)) != NULL)\n> \t\t\treference_rev = lookup_commit_reference(sha1);\n"},{"id":"261828","messageId":"20150521181429.GA6684@peff.net","threadId":"39390","inReplyTo":"xmqqbnhdkdne.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T18:14:30Z","receivedAt":"2015-05-21T18:14:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2015 at 11:07:33AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > All of the information needed to find the @{upstream} of a\n> > branch is included in the branch struct, but callers have to\n> > navigate a series of possible-NULL values to get there.\n> > Let's wrap that logic up in an easy-to-read helper.\n> >\n> > Signed-off-by: Jeff King \n> \n> This step in the whole series is a gem. I cannot believe that we\n> were content having to repeat that \"branch->merge[0]->dst if we can\n> dereference down to that level\" this many times. Nice clean-up.\n\nThere is a related cleanup I resisted, which is that several call-sites\nwill call stat_tracking_info, then later look directly at\nbranch->merge[0]->dst without a check for NULL (fill_tracking_info is\nsuch a site).\n\nThis works because stat_tracking_info's return value tells us that we\ndid indeed find an upstream to compare with. But it feels a little leaky\nto me. One solution is for stat_tracking_info to pass out the name of\nthte upstream, making the caller side something like:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cc55ff2..edc4deb 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -425,11 +425,12 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \tint ours, theirs;\n \tchar *ref = NULL;\n \tstruct branch *branch = branch_get(branch_name);\n+\tconst char *upstream;\n \tstruct strbuf fancy = STRBUF_INIT;\n \tint upstream_is_gone = 0;\n \tint added_decoration = 1;\n \n-\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n+\tswitch (stat_tracking_info(branch, &ours, &theirs, &upstream)) {\n \tcase 0:\n \t\t/* no base */\n \t\treturn;\n@@ -443,7 +444,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \t}\n \n \tif (show_upstream_ref) {\n-\t\tref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\t\tref = shorten_unambiguous_ref(upstream, 0);\n \t\tif (want_color(branch_use_color))\n \t\t\tstrbuf_addf(&fancy, \"%s%s%s\",\n \t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM),\n\n\nThis is still a little error-prone, though. We assume \"upstream\" was\nfilled in depending on the return value of stat_tracking_info. I wonder\nif we could get rid of the weird tri-state return value from\nstat_tracking_info, and just have callers detect the \"there is no base\"\ncase by checking \"upstream != NULL\".\n\nI dunno. It is not buggy in any of the current callers, so it might not\nbe worth spending too much time on.\n\n-Peff\n"},{"id":"261830","messageId":"xmqq7fs1kcfd.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521044532.GH23409@peff.net","subject":"Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T18:33:58Z","receivedAt":"2015-05-21T18:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/remote.c b/remote.c\n> index dca3442..1b7051a 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1705,10 +1705,35 @@ int branch_merge_matches(struct branch *branch,\n> \treturn refname_match(branch->merge[i]->src, refname);\n> }\n> \n> -const char *branch_get_upstream(struct branch *branch)\n> +__attribute((format (printf,2,3)))\n> +static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n> {\n> -\tif (!branch || !branch->merge || !branch->merge[0])\n> -\t\treturn NULL;\n> +\tif (err) {\n> +\t\tva_list ap;\n> +\t\tva_start(ap, fmt);\n> +\t\tstrbuf_vaddf(err, fmt, ap);\n> +\t\tva_end(ap);\n> +\t}\n> +\treturn NULL;\n> +}\n\nMany of our functions return -1 to signal an error, and that is why\nit makes sense for our error() helper to return -1 to save code in\nthe caller, but only because the callers of this private helper use\na NULL to signal an error, this also returns NULL. If we were to\nuse the \"callers can opt into detailed message by passing strbuf\"\npattern more widely, we would want a variant of the above that\nreturns -1, too. And such a helper would do the same thing as\nabove, with only difference from the above is to return -1.\n\nIt's a shame that we have to return something from this function,\nwhose primary purpose is \"we may or may not want an error message in\na strbuf, so format the message when and only when we give you a\nstrbuf\", but C forces us to make it \"always return NULL to signal an\nerror to the caller, and optionally format the message into a strbuf\nif given\".\n\nAnd the name of this helper function only captures the \"optionally\nformat the message\" part, not the \"always return NULL\" part.\n\n> +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n> +{\n> +\tif (!branch)\n> +\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> +\tif (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {\n> +\t\tif (!ref_exists(branch->refname))\n> +\t\t\treturn error_buf(err, _(\"no such branch: '%s'\"),\n> +\t\t\t\t\t branch->name);\n> +\t\tif (!branch->merge)\n> +\t\t\treturn error_buf(err,\n> +\t\t\t\t\t _(\"no upstream configured for branch '%s'\"),\n> +\t\t\t\t\t branch->name);\n> +\t\treturn error_buf(err,\n> +\t\t\t\t _(\"upstream branch '%s' not stored as a remote-tracking branch\"),\n> +\t\t\t\t branch->merge[0]->src);\n> +\t}\n> +\n> \treturn branch->merge[0]->dst;\n> }\n\nThis is a faithful conversion of what the get_upstream_branch() used\nto do, but that ref_exists() check and the error checking there look\nsomewhat out of place.\n\nIt makes the reader wonder what should happen when \"branch->refname\"\ndoes not exist as a ref, but \"branch->merge[0]->dst\" can be fully\ndereferenced. Should it be an error, or if it is OK, the reason why\nit is OK is...?\n"},{"id":"261831","messageId":"20150521183500.GA8988@peff.net","threadId":"39390","inReplyTo":"20150521181429.GA6684@peff.net","subject":"Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T18:35:00Z","receivedAt":"2015-05-21T18:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2015 at 02:14:29PM -0400, Jeff King wrote:\n\n> There is a related cleanup I resisted, which is that several call-sites\n> will call stat_tracking_info, then later look directly at\n> branch->merge[0]->dst without a check for NULL (fill_tracking_info is\n> such a site).\n> \n> This works because stat_tracking_info's return value tells us that we\n> did indeed find an upstream to compare with. But it feels a little leaky\n> to me. One solution is for stat_tracking_info to pass out the name of\n> thte upstream, making the caller side something like:\n\nSo just for fun, here is what a whole patch, with the refactoring of the\nreturn value, would look like.\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cc55ff2..8ecabd1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \tint ours, theirs;\n \tchar *ref = NULL;\n \tstruct branch *branch = branch_get(branch_name);\n+\tconst char *upstream;\n \tstruct strbuf fancy = STRBUF_INIT;\n \tint upstream_is_gone = 0;\n \tint added_decoration = 1;\n \n-\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\treturn;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {\n+\t\tif (!upstream)\n+\t\t\treturn;\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n \tif (show_upstream_ref) {\n-\t\tref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\t\tref = shorten_unambiguous_ref(upstream, 0);\n \t\tif (want_color(branch_use_color))\n \t\t\tstrbuf_addf(&fancy, \"%s%s%s\",\n \t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM),\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 6847400..05dd23d 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -730,7 +730,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tchar buf[40];\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t &num_theirs) != 1)\n+\t\t\t\t\t\t &num_theirs, NULL))\n \t\t\t\t\tcontinue;\n \n \t\t\t\tif (!num_ours && !num_theirs)\n@@ -753,7 +753,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tassert(branch);\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t\t&num_theirs) != 1)\n+\t\t\t\t\t\t\t&num_theirs, NULL))\n \t\t\t\t\tcontinue;\n \n \t\t\t\tif (!num_ours && !num_theirs)\ndiff --git a/remote.c b/remote.c\nindex be45a39..6765051 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2016,12 +2016,15 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n \n /*\n * Compare a branch with its upstream, and save their differences (number\n- * of commits) in *num_ours and *num_theirs.\n+ * of commits) in *num_ours and *num_theirs. The name of the upstream branch\n+ * (or NULL if no upstream is defined) is returned via *upstream_name, if it\n+ * is not itself NULL.\n *\n- * Return 0 if branch has no upstream (no base), -1 if upstream is missing\n- * (with \"gone\" base), otherwise 1 (with base).\n+ * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no\n+ * upstream defined, or ref does not exist), 0 otherwise.\n */\n-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n+int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n+\t\t const char **upstream_name)\n {\n \tunsigned char sha1[20];\n \tstruct commit *ours, *theirs;\n@@ -2032,8 +2035,10 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \n \t/* Cannot stat unless we are marked to build on top of somebody else. */\n \tbase = branch_get_upstream(branch, NULL);\n+\tif (upstream_name)\n+\t\t*upstream_name = base;\n \tif (!base)\n-\t\treturn 0;\n+\t\treturn -1;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n \tif (read_ref(base, sha1))\n@@ -2051,7 +2056,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \t/* are we the same? */\n \tif (theirs == ours) {\n \t\t*num_theirs = *num_ours = 0;\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n \n \t/* Run \"rev-list --left-right ours...theirs\" internally... */\n@@ -2087,7 +2092,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \t/* clear object flags smudged by the above traversal */\n \tclear_commit_marks(ours, ALL_REV_FLAGS);\n \tclear_commit_marks(theirs, ALL_REV_FLAGS);\n-\treturn 1;\n+\treturn 0;\n }\n \n /*\n@@ -2096,23 +2101,17 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n int format_tracking_info(struct branch *branch, struct strbuf *sb)\n {\n \tint ours, theirs;\n+\tconst char *full_base;\n \tchar *base;\n \tint upstream_is_gone = 0;\n \n-\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\treturn 0;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {\n+\t\tif (!full_base)\n+\t\t\treturn 0;\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n-\tbase = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\tbase = shorten_unambiguous_ref(full_base, 0);\n \tif (upstream_is_gone) {\n \t\tstrbuf_addf(sb,\n \t\t\t_(\"Your branch is based on '%s', but the upstream is gone.\\n\"),\ndiff --git a/remote.h b/remote.h\nindex 3326f2b..312b7ca 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -249,7 +249,8 @@ enum match_refs_flags {\n };\n \n /* Reporting of tracking info */\n-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);\n+int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n+\t\t const char **upstream_name);\n int format_tracking_info(struct branch *branch, struct strbuf *sb);\n \n struct ref *get_local_heads(void);\ndiff --git a/wt-status.c b/wt-status.c\nindex 38cb165..7c8ae57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \n \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n \n-\tswitch (stat_tracking_info(branch, &num_ours, &num_theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n-\t\treturn;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {\n+\t\tif (!base) {\n+\t\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n-\tbase = branch->merge[0]->dst;\n \tbase = shorten_unambiguous_ref(base, 0);\n \tcolor_fprintf(s->fp, header_color, \"...\");\n \tcolor_fprintf(s->fp, branch_color_remote, \"%s\", base);\n"},{"id":"261832","messageId":"xmqq382pkc8u.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521045233.GA26507@peff.net","subject":"Re: [PATCH v3 0/14] implement @{push} shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T18:37:53Z","receivedAt":"2015-05-21T18:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 21, 2015 at 12:44:29AM -0400, Jeff King wrote:\n>\n>> This is a re-roll of the series at:\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/268185\n>> \n>> The only changes here are the addition of patches 2 and 6, which are\n>> both cleanups that help make the other patches more readable/sensible.\n>> They are the same as what was posted during review of the thread linked\n>> above. So there's nothing new here, but of course fresh reviews are\n>> welcome.\n>\n> Actually, that's not quite true; I forgot to mention one change:\n>\n>> [03/14]: remote.c: drop \"remote\" pointer from \"struct branch\"\n>\n> In this version, this one remembers to also update the API\n> documentation.\n>\n> -Peff\n\nThanks for a pleasant read.\nWill queue.\n"},{"id":"261835","messageId":"20150521184910.GA3490@peff.net","threadId":"39390","inReplyTo":"xmqq7fs1kcfd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-21T18:49:10Z","receivedAt":"2015-05-21T18:49:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2015 at 11:33:58AM -0700, Junio C Hamano wrote:\n\n> > +static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n> > {\n> > -\tif (!branch || !branch->merge || !branch->merge[0])\n> > -\t\treturn NULL;\n> > +\tif (err) {\n> > +\t\tva_list ap;\n> > +\t\tva_start(ap, fmt);\n> > +\t\tstrbuf_vaddf(err, fmt, ap);\n> > +\t\tva_end(ap);\n> > +\t}\n> > +\treturn NULL;\n> > +}\n> \n> Many of our functions return -1 to signal an error, and that is why\n> it makes sense for our error() helper to return -1 to save code in\n> the caller, but only because the callers of this private helper use\n> a NULL to signal an error, this also returns NULL. If we were to\n> use the \"callers can opt into detailed message by passing strbuf\"\n> pattern more widely, we would want a variant of the above that\n> returns -1, too. And such a helper would do the same thing as\n> above, with only difference from the above is to return -1.\n\nYeah, this is not really a good general-purpose purpose function in that\nsense. I have often wanted an error() that returns NULL, but avoided\nadding just because it seemed like needless proliferation.\n\nThe real reason to include the return value at all in error() is to let\nyou turn two-liners into one-liners. We could drop the return value from\nthis helper entirely (or make it -1, but of course no callers would use\nit) and write it out long-hand in the callers:\n\n if (!branch->merge) {\n\terror_buf(err, ...);\n\treturn NULL;\n }\n\nThat is really not so bad, as there are only a handful of callers.\n\n> > +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n> > +{\n> > +\tif (!branch)\n> > +\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> > +\tif (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {\n> > +\t\tif (!ref_exists(branch->refname))\n> > +\t\t\treturn error_buf(err, _(\"no such branch: '%s'\"),\n> > +\t\t\t\t\t branch->name);\n> > +\t\tif (!branch->merge)\n> > +\t\t\treturn error_buf(err,\n> > +\t\t\t\t\t _(\"no upstream configured for branch '%s'\"),\n> > +\t\t\t\t\t branch->name);\n> > +\t\treturn error_buf(err,\n> > +\t\t\t\t _(\"upstream branch '%s' not stored as a remote-tracking branch\"),\n> > +\t\t\t\t branch->merge[0]->src);\n> > +\t}\n> > +\n> > \treturn branch->merge[0]->dst;\n> > }\n> \n> This is a faithful conversion of what the get_upstream_branch() used\n> to do, but that ref_exists() check and the error checking there look\n> somewhat out of place.\n> \n> It makes the reader wonder what should happen when \"branch->refname\"\n> does not exist as a ref, but \"branch->merge[0]->dst\" can be fully\n> dereferenced. Should it be an error, or if it is OK, the reason why\n> it is OK is...?\n\nYeah, my goal here was to faithfully keep the same logic, but I had a\nsimilar head-scratching moment. What would make more sense to me is:\n\n if (!branch)\n\treturn error(\"HEAD does not point to a branch\");\n\n if (!branch->merge || !branch->merge[0]) {\n\t/*\n\t * no merge config; is it because the user didn't define any, or\n\t * because it is not a real branch, and get_branch auto-vivified\n\t * it?\n\t */\n\tif (!ref_exists(branch->refname))\n\t\treturn error(\"no such branch\");\n\treturn error(\"no upstream configured for branch\");\n }\n\n if (!branch->merge[0]->dst)\n\treturn error(\"upstream branch not stored as a remote-tracking branch\");\n\n return branch->merge[0]->dst;\n\n\nHopefully the comment there answers your question; it is not that we\ntruly care whether the ref exists, but only that we are trying to tell\nthe difference between a \"real\" branch and a struct that is an artifact\nof our internal code.\n\nNote also that the original may dereference branch->merge[0] even if it\nis NULL. I think that can't actually happen in practice (we only\nallocate branch->merge if we have at least one item to put in it, and\nall of the checks of branch->merge[0] are actually being over-careful).\nBut the code I just wrote above does not have that problem.\n\n-Peff\n"},{"id":"261839","messageId":"xmqqtwv5ivvf.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521183500.GA8988@peff.net","subject":"Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T19:16:52Z","receivedAt":"2015-05-21T19:16:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 21, 2015 at 02:14:29PM -0400, Jeff King wrote:\n>\n>> There is a related cleanup I resisted, which is that several call-sites\n>> will call stat_tracking_info, then later look directly at\n>> branch->merge[0]->dst without a check for NULL (fill_tracking_info is\n>> such a site).\n>> \n>> This works because stat_tracking_info's return value tells us that we\n>> did indeed find an upstream to compare with. But it feels a little leaky\n>> to me. One solution is for stat_tracking_info to pass out the name of\n>> thte upstream, making the caller side something like:\n>\n> So just for fun, here is what a whole patch, with the refactoring of the\n> return value, would look like.\n\nIt looks much nicer and reads better, at least to me.\n\nNice.\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index cc55ff2..8ecabd1 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n> \tint ours, theirs;\n> \tchar *ref = NULL;\n> \tstruct branch *branch = branch_get(branch_name);\n> +\tconst char *upstream;\n> \tstruct strbuf fancy = STRBUF_INIT;\n> \tint upstream_is_gone = 0;\n> \tint added_decoration = 1;\n> \n> -\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n> -\tcase 0:\n> -\t\t/* no base */\n> -\t\treturn;\n> -\tcase -1:\n> -\t\t/* with \"gone\" base */\n> +\tif (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {\n> +\t\tif (!upstream)\n> +\t\t\treturn;\n> \t\tupstream_is_gone = 1;\n> -\t\tbreak;\n> -\tdefault:\n> -\t\t/* with base */\n> -\t\tbreak;\n> \t}\n> \n> \tif (show_upstream_ref) {\n> -\t\tref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n> +\t\tref = shorten_unambiguous_ref(upstream, 0);\n> \t\tif (want_color(branch_use_color))\n> \t\t\tstrbuf_addf(&fancy, \"%s%s%s\",\n> \t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM),\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 6847400..05dd23d 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -730,7 +730,7 @@ static void populate_value(struct refinfo *ref)\n> \t\t\t\tchar buf[40];\n> \n> \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n> -\t\t\t\t\t\t &num_theirs) != 1)\n> +\t\t\t\t\t\t &num_theirs, NULL))\n> \t\t\t\t\tcontinue;\n> \n> \t\t\t\tif (!num_ours && !num_theirs)\n> @@ -753,7 +753,7 @@ static void populate_value(struct refinfo *ref)\n> \t\t\t\tassert(branch);\n> \n> \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n> -\t\t\t\t\t\t\t&num_theirs) != 1)\n> +\t\t\t\t\t\t\t&num_theirs, NULL))\n> \t\t\t\t\tcontinue;\n> \n> \t\t\t\tif (!num_ours && !num_theirs)\n> diff --git a/remote.c b/remote.c\n> index be45a39..6765051 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2016,12 +2016,15 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n> \n> /*\n> * Compare a branch with its upstream, and save their differences (number\n> - * of commits) in *num_ours and *num_theirs.\n> + * of commits) in *num_ours and *num_theirs. The name of the upstream branch\n> + * (or NULL if no upstream is defined) is returned via *upstream_name, if it\n> + * is not itself NULL.\n> *\n> - * Return 0 if branch has no upstream (no base), -1 if upstream is missing\n> - * (with \"gone\" base), otherwise 1 (with base).\n> + * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no\n> + * upstream defined, or ref does not exist), 0 otherwise.\n> */\n> -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n> +\t\t const char **upstream_name)\n> {\n> \tunsigned char sha1[20];\n> \tstruct commit *ours, *theirs;\n> @@ -2032,8 +2035,10 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n> \n> \t/* Cannot stat unless we are marked to build on top of somebody else. */\n> \tbase = branch_get_upstream(branch, NULL);\n> +\tif (upstream_name)\n> +\t\t*upstream_name = base;\n> \tif (!base)\n> -\t\treturn 0;\n> +\t\treturn -1;\n> \n> \t/* Cannot stat if what we used to build on no longer exists */\n> \tif (read_ref(base, sha1))\n> @@ -2051,7 +2056,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n> \t/* are we the same? */\n> \tif (theirs == ours) {\n> \t\t*num_theirs = *num_ours = 0;\n> -\t\treturn 1;\n> +\t\treturn 0;\n> \t}\n> \n> \t/* Run \"rev-list --left-right ours...theirs\" internally... */\n> @@ -2087,7 +2092,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n> \t/* clear object flags smudged by the above traversal */\n> \tclear_commit_marks(ours, ALL_REV_FLAGS);\n> \tclear_commit_marks(theirs, ALL_REV_FLAGS);\n> -\treturn 1;\n> +\treturn 0;\n> }\n> \n> /*\n> @@ -2096,23 +2101,17 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n> int format_tracking_info(struct branch *branch, struct strbuf *sb)\n> {\n> \tint ours, theirs;\n> +\tconst char *full_base;\n> \tchar *base;\n> \tint upstream_is_gone = 0;\n> \n> -\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n> -\tcase 0:\n> -\t\t/* no base */\n> -\t\treturn 0;\n> -\tcase -1:\n> -\t\t/* with \"gone\" base */\n> +\tif (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {\n> +\t\tif (!full_base)\n> +\t\t\treturn 0;\n> \t\tupstream_is_gone = 1;\n> -\t\tbreak;\n> -\tdefault:\n> -\t\t/* with base */\n> -\t\tbreak;\n> \t}\n> \n> -\tbase = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n> +\tbase = shorten_unambiguous_ref(full_base, 0);\n> \tif (upstream_is_gone) {\n> \t\tstrbuf_addf(sb,\n> \t\t\t_(\"Your branch is based on '%s', but the upstream is gone.\\n\"),\n> diff --git a/remote.h b/remote.h\n> index 3326f2b..312b7ca 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -249,7 +249,8 @@ enum match_refs_flags {\n> };\n> \n> /* Reporting of tracking info */\n> -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);\n> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n> +\t\t const char **upstream_name);\n> int format_tracking_info(struct branch *branch, struct strbuf *sb);\n> \n> struct ref *get_local_heads(void);\n> diff --git a/wt-status.c b/wt-status.c\n> index 38cb165..7c8ae57 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n> \n> \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n> \n> -\tswitch (stat_tracking_info(branch, &num_ours, &num_theirs)) {\n> -\tcase 0:\n> -\t\t/* no base */\n> -\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n> -\t\treturn;\n> -\tcase -1:\n> -\t\t/* with \"gone\" base */\n> +\tif (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {\n> +\t\tif (!base) {\n> +\t\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> \t\tupstream_is_gone = 1;\n> -\t\tbreak;\n> -\tdefault:\n> -\t\t/* with base */\n> -\t\tbreak;\n> \t}\n> \n> -\tbase = branch->merge[0]->dst;\n> \tbase = shorten_unambiguous_ref(base, 0);\n> \tcolor_fprintf(s->fp, header_color, \"...\");\n> \tcolor_fprintf(s->fp, branch_color_remote, \"%s\", base);\n"},{"id":"261840","messageId":"xmqqpp5tivga.fsf@gitster.dls.corp.google.com","threadId":"39390","inReplyTo":"20150521184910.GA3490@peff.net","subject":"Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-21T19:25:57Z","receivedAt":"2015-05-21T19:25:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 21, 2015 at 11:33:58AM -0700, Junio C Hamano wrote:\n>\n>> > +static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n>> > {\n>> > -\tif (!branch || !branch->merge || !branch->merge[0])\n>> > -\t\treturn NULL;\n>> > +\tif (err) {\n>> > +\t\tva_list ap;\n>> > +\t\tva_start(ap, fmt);\n>> > +\t\tstrbuf_vaddf(err, fmt, ap);\n>> > +\t\tva_end(ap);\n>> > +\t}\n>> > +\treturn NULL;\n>> > +}\n>> \n>> Many of our functions return -1 to signal an error, and that is why\n>> it makes sense for our error() helper to return -1 to save code in\n>> the caller, but only because the callers of this private helper use\n>> a NULL to signal an error, this also returns NULL. If we were to\n>> use the \"callers can opt into detailed message by passing strbuf\"\n>> pattern more widely, we would want a variant of the above that\n>> returns -1, too. And such a helper would do the same thing as\n>> above, with only difference from the above is to return -1.\n>\n> Yeah, this is not really a good general-purpose purpose function in that\n> sense. I have often wanted an error() that returns NULL, but avoided\n> adding just because it seemed like needless proliferation.\n>\n> The real reason to include the return value at all in error() is to let\n> you turn two-liners into one-liners.\n\nYeah, our error() returns -1 to save code in the caller. And the\ncallers of this private helper all want to return NULL, so this\nreturns NULL for the same reason.\n\n> We could drop the return value from\n> this helper entirely (or make it -1, but of course no callers would use\n> it) and write it out long-hand in the callers:\n>\n> if (!branch->merge) {\n> \terror_buf(err, ...);\n> \treturn NULL;\n> }\n>\n> That is really not so bad, as there are only a handful of callers.\n\nThat may happen when somebody (perhaps Jonathan?) wants to push the\n\"let's optionally pass strbuf to format messages into\" approach\nforward, and most likely be done by adding a similar function to\nusage.c next to error_builtin() and friends. As long as we do not\nforget to reimplement this helper in terms of that public function\nwhen it happens, what we have right now after this patch would be\nfine.\n\n> Yeah, my goal here was to faithfully keep the same logic, but I had a\n> similar head-scratching moment. What would make more sense to me is:\n>\n> if (!branch)\n> \treturn error(\"HEAD does not point to a branch\");\n>\n> if (!branch->merge || !branch->merge[0]) {\n> \t/*\n> \t * no merge config; is it because the user didn't define any, or\n> \t * because it is not a real branch, and get_branch auto-vivified\n> \t * it?\n> \t */\n> \tif (!ref_exists(branch->refname))\n> \t\treturn error(\"no such branch\");\n> \treturn error(\"no upstream configured for branch\");\n> }\n>\n> if (!branch->merge[0]->dst)\n> \treturn error(\"upstream branch not stored as a remote-tracking branch\");\n>\n> return branch->merge[0]->dst;\n>\n> Hopefully the comment there answers your question; it is not that we\n> truly care whether the ref exists, but only that we are trying to tell\n> the difference between a \"real\" branch and a struct that is an artifact\n> of our internal code.\n\nWell, answering \"my\" question here on the list wouldn't help future\nreaders of the code very much ;-)\n\n> Note also that the original may dereference branch->merge[0] even if it\n> is NULL. I think that can't actually happen in practice (we only\n> allocate branch->merge if we have at least one item to put in it, and\n> all of the checks of branch->merge[0] are actually being over-careful).\n> But the code I just wrote above does not have that problem.\n\nPerhaps update the patch with this explanation in the log message,\nas a separate preparatory step?\n\nThanks.\n"},{"id":"261868","messageId":"20150522004643.GA11739@peff.net","threadId":"39390","inReplyTo":"xmqqpp5tivga.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T00:46:43Z","receivedAt":"2015-05-22T00:46:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:\n\n> > Note also that the original may dereference branch->merge[0] even if it\n> > is NULL. I think that can't actually happen in practice (we only\n> > allocate branch->merge if we have at least one item to put in it, and\n> > all of the checks of branch->merge[0] are actually being over-careful).\n> > But the code I just wrote above does not have that problem.\n> \n> Perhaps update the patch with this explanation in the log message,\n> as a separate preparatory step?\n\nI decided on a separate patch on top which improves the logic and\nexplains the issues. Here it is (it goes on top of the existing patch 8,\n\"report specific errors from branch_get_upstream\").\n\n-- >8 --\nSubject: remote.c: untangle error logic in branch_get_upstream\n\nThe error-diagnosis logic in branch_get_upstream was copied\nstraight from sha1_name.c in the previous commit. However,\nbecause we check all error cases and upfront and then later\ndiagnose them, the logic is a bit tangled. In particular:\n\n - if branch->merge[0] is NULL, we may end up dereferencing\n it for an error message (in practice, it should never be\n NULL, so this is probably not a triggerable bug).\n\n - We may enter the code path because branch->merge[0]->dst\n is NULL, but we then start our error diagnosis by\n checking whether our local branch exists. But that is\n only relevant to diagnosing missing merge config, not a\n missing tracking ref; our diagnosis may hide the real\n problem.\n\nInstead, let's just use a sequence of \"if\" blocks to check\nfor each error type, diagnose it, and return NULL.\n\nSigned-off-by: Jeff King \n---\n remote.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 1b7051a..d2519c2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1721,18 +1721,25 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n {\n \tif (!branch)\n \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n-\tif (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {\n+\n+\tif (!branch->merge || !branch->merge[0]) {\n+\t\t/*\n+\t\t * no merge config; is it because the user didn't define any,\n+\t\t * or because it is not a real branch, and get_branch\n+\t\t * auto-vivified it?\n+\t\t */\n \t\tif (!ref_exists(branch->refname))\n \t\t\treturn error_buf(err, _(\"no such branch: '%s'\"),\n \t\t\t\t\t branch->name);\n-\t\tif (!branch->merge)\n-\t\t\treturn error_buf(err,\n-\t\t\t\t\t _(\"no upstream configured for branch '%s'\"),\n-\t\t\t\t\t branch->name);\n+\t\treturn error_buf(err,\n+\t\t\t\t _(\"no upstream configured for branch '%s'\"),\n+\t\t\t\t branch->name);\n+\t}\n+\n+\tif (!branch->merge[0]->dst)\n \t\treturn error_buf(err,\n \t\t\t\t _(\"upstream branch '%s' not stored as a remote-tracking branch\"),\n \t\t\t\t branch->merge[0]->src);\n-\t}\n \n \treturn branch->merge[0]->dst;\n }\n-- \n2.4.1.528.g00591e3\n"},{"id":"261869","messageId":"20150522004910.GB11739@peff.net","threadId":"39390","inReplyTo":"20150522004643.GA11739@peff.net","subject":"Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T00:49:11Z","receivedAt":"2015-05-22T00:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2015 at 08:46:43PM -0400, Jeff King wrote:\n\n> On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:\n> \n> > > Note also that the original may dereference branch->merge[0] even if it\n> > > is NULL. I think that can't actually happen in practice (we only\n> > > allocate branch->merge if we have at least one item to put in it, and\n> > > all of the checks of branch->merge[0] are actually being over-careful).\n> > > But the code I just wrote above does not have that problem.\n> > \n> > Perhaps update the patch with this explanation in the log message,\n> > as a separate preparatory step?\n> \n> I decided on a separate patch on top which improves the logic and\n> explains the issues. Here it is (it goes on top of the existing patch 8,\n> \"report specific errors from branch_get_upstream\").\n> \n> -- >8 --\n> Subject: remote.c: untangle error logic in branch_get_upstream\n\nAnd then on top of that, we can add in this cleanup I showed earlier.\n\nBoth of these should insert into the series without any trouble, but let\nme know if you run into problems and I can repost the whole thing.\n\n-- >8 --\nSubject: remote.c: return upstream name from stat_tracking_info\n\nAfter calling stat_tracking_info, callers often want to\nprint the name of the upstream branch (in addition to the\ntracking count). To do this, they have to access\nbranch->merge->dst[0] themselves. This is not wrong, as the\nreturn value from stat_tracking_info tells us whether we\nhave an upstream branch or not. But it is a bit leaky, as we\nmake an assumption about how it calculated the upstream\nname.\n\nInstead, let's add an out-parameter that lets the caller\nknow the upstream name we found.\n\nAs a bonus, we can get rid of the unusual tri-state return\nfrom the function. We no longer need to use it to\ndifferentiate between \"no tracking config\" and \"tracking ref\ndoes not exist\" (since you can check the upstream_name for\nthat), so we can just use the usual 0/-1 convention for\nsuccess/error.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 16 +++++-----------\n builtin/for-each-ref.c | 4 ++--\n remote.c | 35 +++++++++++++++++------------------\n remote.h | 3 ++-\n wt-status.c | 18 ++++++------------\n 5 files changed, 32 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cc55ff2..8ecabd1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \tint ours, theirs;\n \tchar *ref = NULL;\n \tstruct branch *branch = branch_get(branch_name);\n+\tconst char *upstream;\n \tstruct strbuf fancy = STRBUF_INIT;\n \tint upstream_is_gone = 0;\n \tint added_decoration = 1;\n \n-\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\treturn;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {\n+\t\tif (!upstream)\n+\t\t\treturn;\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n \tif (show_upstream_ref) {\n-\t\tref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\t\tref = shorten_unambiguous_ref(upstream, 0);\n \t\tif (want_color(branch_use_color))\n \t\t\tstrbuf_addf(&fancy, \"%s%s%s\",\n \t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM),\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 18d209b..92bd2b2 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -716,7 +716,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tchar buf[40];\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t &num_theirs) != 1)\n+\t\t\t\t\t\t &num_theirs, NULL))\n \t\t\t\t\tcontinue;\n \n \t\t\t\tif (!num_ours && !num_theirs)\n@@ -738,7 +738,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tassert(branch);\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t\t&num_theirs) != 1)\n+\t\t\t\t\t\t\t&num_theirs, NULL))\n \t\t\t\t\tcontinue;\n \n \t\t\t\tif (!num_ours && !num_theirs)\ndiff --git a/remote.c b/remote.c\nindex d2519c2..c884574 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1938,12 +1938,15 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n \n /*\n * Compare a branch with its upstream, and save their differences (number\n- * of commits) in *num_ours and *num_theirs.\n+ * of commits) in *num_ours and *num_theirs. The name of the upstream branch\n+ * (or NULL if no upstream is defined) is returned via *upstream_name, if it\n+ * is not itself NULL.\n *\n- * Return 0 if branch has no upstream (no base), -1 if upstream is missing\n- * (with \"gone\" base), otherwise 1 (with base).\n+ * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no\n+ * upstream defined, or ref does not exist), 0 otherwise.\n */\n-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n+int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n+\t\t const char **upstream_name)\n {\n \tunsigned char sha1[20];\n \tstruct commit *ours, *theirs;\n@@ -1954,8 +1957,10 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \n \t/* Cannot stat unless we are marked to build on top of somebody else. */\n \tbase = branch_get_upstream(branch, NULL);\n+\tif (upstream_name)\n+\t\t*upstream_name = base;\n \tif (!base)\n-\t\treturn 0;\n+\t\treturn -1;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n \tif (read_ref(base, sha1))\n@@ -1973,7 +1978,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \t/* are we the same? */\n \tif (theirs == ours) {\n \t\t*num_theirs = *num_ours = 0;\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n \n \t/* Run \"rev-list --left-right ours...theirs\" internally... */\n@@ -2009,7 +2014,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \t/* clear object flags smudged by the above traversal */\n \tclear_commit_marks(ours, ALL_REV_FLAGS);\n \tclear_commit_marks(theirs, ALL_REV_FLAGS);\n-\treturn 1;\n+\treturn 0;\n }\n \n /*\n@@ -2018,23 +2023,17 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n int format_tracking_info(struct branch *branch, struct strbuf *sb)\n {\n \tint ours, theirs;\n+\tconst char *full_base;\n \tchar *base;\n \tint upstream_is_gone = 0;\n \n-\tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\treturn 0;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {\n+\t\tif (!full_base)\n+\t\t\treturn 0;\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n-\tbase = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\tbase = shorten_unambiguous_ref(full_base, 0);\n \tif (upstream_is_gone) {\n \t\tstrbuf_addf(sb,\n \t\t\t_(\"Your branch is based on '%s', but the upstream is gone.\\n\"),\ndiff --git a/remote.h b/remote.h\nindex 03ca005..357a909 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -239,7 +239,8 @@ enum match_refs_flags {\n };\n \n /* Reporting of tracking info */\n-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);\n+int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n+\t\t const char **upstream_name);\n int format_tracking_info(struct branch *branch, struct strbuf *sb);\n \n struct ref *get_local_heads(void);\ndiff --git a/wt-status.c b/wt-status.c\nindex 38cb165..7c8ae57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \n \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n \n-\tswitch (stat_tracking_info(branch, &num_ours, &num_theirs)) {\n-\tcase 0:\n-\t\t/* no base */\n-\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n-\t\treturn;\n-\tcase -1:\n-\t\t/* with \"gone\" base */\n+\tif (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {\n+\t\tif (!base) {\n+\t\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tupstream_is_gone = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\t/* with base */\n-\t\tbreak;\n \t}\n \n-\tbase = branch->merge[0]->dst;\n \tbase = shorten_unambiguous_ref(base, 0);\n \tcolor_fprintf(s->fp, header_color, \"...\");\n \tcolor_fprintf(s->fp, branch_color_remote, \"%s\", base);\n-- \n2.4.1.528.g00591e3\n"}]}