{"thread":{"id":"38961","subject":"[PATCH 0/6] implement @{push} shorthand","startedAt":"2015-03-31T17:33:40Z","lastAt":"2015-03-31T22:57:53Z","messageCount":18,"participants":["Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"258750","messageId":"20150331173339.GA17732@peff.net","threadId":"38961","inReplyTo":null,"subject":"[PATCH 0/6] implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:33:40Z","receivedAt":"2015-03-31T17:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The basic idea is that in a triangular workflow, it's useful to be able\nto refer to both @{upstream} and \"the place I would push to\" with a\nshorthand. This idea was discussed over a year ago:\n\n http://thread.gmane.org/gmane.comp.version-control.git/240144/focus=240198\n\nI've found it useful since then, so I decided to clean it up and submit\nit. This iteration renames it to \"@{push}\" (rather than \"@{publish}\"),\nwhich is more descriptive. It's rebased, as there were some changes in\nthe area, and it also fixes some bugs in the original (of course, I may\nalso have _introduced_ bugs, as this is largely a rewrite that has not\nhad a year's worth of exercise).\n\nI also took the opportunity to do some more aggressive refactoring of\nthe remote.c code, as there were some bits there that confused me every\ntime I looked at it. I think the result is easier to follow.\n\n [1/6]: remote.c: drop default_remote_name variable\n [2/6]: remote.c: drop \"remote\" pointer from \"struct branch\"\n [3/6]: remote.c: hoist branch.*.remote lookup out of remote_get_1\n [4/6]: remote.c: provide per-branch pushremote name\n [5/6]: sha1_name: refactor upstream_mark\n [6/6]: sha1_name: implement @{push} shorthand\n\nThe one thing I _didn't_ do here is refactor the logic in\nbuiltin/push.c so what we could reuse it. There's still a switch\nstatement here on push_default. I don't like that, but I spent an hour\nor so trying to pull out the push.c logic, and it got rather nasty.\nThere are a lot of global variables that get mutated by one-off\nfunctions, and it all needs to happen in the right order. It looked like\nany serious refactoring there is going to have a risk of regressions in\npush.\n\nSo I punted on that. The simplified logic that is here in sha1_name is\nnot too bad, and I feel like we can unify the two at a later date if we\nwant to. And I'd much rather see bug reports like \"@{push} doesn't act\nlike git-push\" and deal with that, as opposed to \"your series breaks\npush\".\n\n-Peff\n"},{"id":"258751","messageId":"20150331173437.GA18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 1/6] remote.c: drop default_remote_name variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:34:38Z","receivedAt":"2015-03-31T17:34:38Z","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 remotes and push-remotes matches.\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 | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 68901b0..fcd868d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -39,6 +39,8 @@ struct rewrites {\n \tint rewrite_nr;\n };\n \n+static int loaded_remotes_config;\n+\n static struct remote **remotes;\n static int remotes_alloc;\n static int remotes_nr;\n@@ -49,10 +51,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 +367,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@@ -504,9 +499,11 @@ static void read_config(void)\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_remotes_config)\n \t\treturn;\n-\tdefault_remote_name = \"origin\";\n+\tloaded_remotes_config = 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 +705,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.0.rc0.363.gf9f328b\n"},{"id":"258752","messageId":"20150331173555.GB18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 2/6] remote.c: drop \"remote\" pointer from \"struct branch\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:35:55Z","receivedAt":"2015-03-31T17:35:55Z","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 based on that name. However, it turns\nout that nobody really cares about this field. The only two\nsites that access it are:\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\" for\n that; we can just look it up for the duration of the\n operation.\n\nGetting rid of it drops one potential source of confusion:\nis the value the match for \"remote_name\", or is it the\nremote we would fetch from when on that branch (i.e., does\nit fall back to \"origin\")?\n\nWhen we add pushremote_name, this question would get even\nmore confusing, as pushremotes have a more complicated\nlookup procedure. It would be nice for the code to be\nconsistent between the remote and pushremote, and this takes\nus one step closer.\n\nSigned-off-by: Jeff King \n---\n builtin/merge.c | 2 +-\n remote.c | 14 ++++++++------\n remote.h | 1 -\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3b0f8f9..1840317 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -955,7 +955,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 fcd868d..d5fd605 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1633,15 +1633,20 @@ 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 \n+\tif (!ret->remote_name || !ret->merge_nr)\n+\t\treturn;\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@@ -1661,11 +1666,8 @@ 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-\t\tif (ret->merge_nr)\n-\t\t\tset_merge(ret);\n-\t}\n+\tif (ret)\n+\t set_merge(ret);\n \treturn ret;\n }\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.0.rc0.363.gf9f328b\n"},{"id":"258754","messageId":"20150331173613.GC18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 3/6] remote.c: hoist branch.*.remote lookup out of remote_get_1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:36:13Z","receivedAt":"2015-03-31T17:36:13Z","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 d5fd605..9555904 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -693,6 +693,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@@ -704,13 +716,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.0.rc0.363.gf9f328b\n"},{"id":"258755","messageId":"20150331173727.GD18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 4/6] remote.c: provide per-branch pushremote name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:37:28Z","receivedAt":"2015-03-31T17:37:28Z","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 | 41 +++++++++++++++++++++++------------------\n remote.h | 2 ++\n 2 files changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9555904..8363bdb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -51,7 +51,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@@ -369,9 +368,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@@ -511,10 +508,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@@ -705,20 +698,32 @@ 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, int for_push)\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 = for_push ?\n+\t\t pushremote_for_branch(current_branch, &name_given) :\n+\t\t remote_for_branch(current_branch, &name_given);\n \n \tret = make_remote(name, 0);\n \tif (valid_remote_nick(name)) {\n@@ -739,13 +744,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, 0);\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, 1);\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.0.rc0.363.gf9f328b\n"},{"id":"258756","messageId":"20150331173740.GE18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 5/6] sha1_name: refactor upstream_mark","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:37:41Z","receivedAt":"2015-03-31T17:37:41Z","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 6d10f05..3741ca3 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.0.rc0.363.gf9f328b\n"},{"id":"258757","messageId":"20150331173834.GF18912@peff.net","threadId":"38961","inReplyTo":"20150331173339.GA17732@peff.net","subject":"[PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T17:38:35Z","receivedAt":"2015-03-31T17:38:35Z","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\nSigned-off-by: Jeff King \n---\nAs an aside, I messed up an &&-chain in the tests, which was caught by\nGIT_TEST_CHAIN_LINT before submitting. The system works!\n\n Documentation/revisions.txt | 25 +++++++++++\n sha1_name.c | 102 +++++++++++++++++++++++++++++++++++++++++++-\n t/t1514-rev-parse-push.sh | 63 +++++++++++++++++++++++++++\n 3 files changed, 189 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..5d9df25 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` is 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 3741ca3..1da3291 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@@ -1104,6 +1111,95 @@ static int interpret_upstream_mark(const char *name, int namelen,\n \treturn len + at;\n }\n \n+static char *tracking_ref_for(struct remote *remote, const char *refname)\n+{\n+\tchar *ret;\n+\n+\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n+\tif (!ret)\n+\t\tdie(_(\"@{push} has no local tracking branch for remote '%s'\"),\n+\t\t refname);\n+\treturn ret;\n+}\n+\n+static char *get_push_branch(const char *name, int len)\n+{\n+\tchar *branch_name;\n+\tstruct branch *branch;\n+\tstruct remote *remote;\n+\n+\tbranch_name = xmemdupz(name, len);\n+\tbranch = branch_get(*branch_name ? branch_name : NULL);\n+\tif (!branch)\n+\t\tdie(_(\"HEAD does not point to a branch\"));\n+\tfree(branch_name);\n+\n+\tremote = remote_get(pushremote_for_branch(branch, NULL));\n+\tif (!remote)\n+\t\tdie(_(\"branch '%s' has no remote for pushing\"), branch->name);\n+\n+\tif (remote->push_refspec_nr) {\n+\t\tchar *dst, *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\tdie(_(\"push refspecs for '%s' do not include '%s'\"),\n+\t\t\t remote->name, branch->name);\n+\n+\t\tret = tracking_ref_for(remote, dst);\n+\t\tfree(dst);\n+\t\treturn ret;\n+\t}\n+\n+\tif (remote->mirror)\n+\t\treturn tracking_ref_for(remote, branch->refname);\n+\n+\tswitch (push_default) {\n+\tcase PUSH_DEFAULT_NOTHING:\n+\t\tdie(_(\"@{push} has no destination (push.default is 'nothing'\"));\n+\n+\tcase PUSH_DEFAULT_MATCHING:\n+\tcase PUSH_DEFAULT_CURRENT:\n+\t\treturn tracking_ref_for(remote, branch->refname);\n+\n+\tcase PUSH_DEFAULT_UPSTREAM:\n+\t\treturn xstrdup(get_upstream_branch(name, len));\n+\n+\tcase PUSH_DEFAULT_UNSPECIFIED:\n+\tcase PUSH_DEFAULT_SIMPLE:\n+\t\t{\n+\t\t\tconst char *up = get_upstream_branch(name, len);\n+\t\t\tchar *cur = tracking_ref_for(remote, branch->refname);\n+\t\t\tif (strcmp(cur, up))\n+\t\t\t\tdie(\"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+static int interpret_push_mark(const char *name, int namelen,\n+\t\t\t int at, struct strbuf *buf)\n+{\n+\tint len;\n+\tchar *result;\n+\n+\tlen = push_mark(name + at, namelen - at);\n+\tif (!len)\n+\t\treturn -1;\n+\n+\tif (memchr(name, ':', at))\n+\t\treturn -1;\n+\n+\tresult = get_push_branch(name, at);\n+\tset_shortened_ref(buf, result);\n+\tfree(result);\n+\n+\treturn len + at;\n+}\n+\n /*\n * This reads short-hand syntax that not only evaluates to a commit\n * object name, but also can act as if the end user spelled the name\n@@ -1154,6 +1250,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\tlen = interpret_upstream_mark(name, namelen, at - name, buf);\n \t\tif (len > 0)\n \t\t\treturn len;\n+\n+\t\tlen = interpret_push_mark(name, namelen, at - name, buf);\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.0.rc0.363.gf9f328b\n"},{"id":"258772","messageId":"xmqqzj6sq4xc.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331173437.GA18912@peff.net","subject":"Re: [PATCH 1/6] remote.c: drop default_remote_name variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T20:37:35Z","receivedAt":"2015-03-31T20:37:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we read the remote config from disk, we update a\n> default_remote_name variable if we see branch.*.remote\n> config for the current branch. This isn't wrong, or even all\n> that complicated, but it is a bit simpler (because it\n> reduces our overall state) to just lazily compute the\n> default when we need it.\n>\n> The ulterior motive here is that the push config uses a\n> similar structure, and _is_ much more complicated as a\n> result. That will be simplified in a future patch, and it's\n> more readable if the logic remotes and push-remotes matches.\n\nI cannot quite parse the part after \"if...\"; \"the logic used by\nremotes and push-remotes matches\"?\n\n> Note that we also used default_remote_name as a signal that\n> the remote config has been loaded; after this patch, we now\n> use an explicit flag.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote.c | 24 ++++++++++++------------\n> 1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 68901b0..fcd868d 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -39,6 +39,8 @@ struct rewrites {\n> \tint rewrite_nr;\n> };\n> \n> +static int loaded_remotes_config;\n> +\n\nI expect that the reason why this is not a function scope static in\nread_config() will be revealed in the later patch in this series...\n"},{"id":"258773","messageId":"xmqqvbhgq4ci.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331173555.GB18912@peff.net","subject":"Re: [PATCH 2/6] remote.c: drop \"remote\" pointer from \"struct branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T20:50:05Z","receivedAt":"2015-03-31T20:50:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we create each branch struct, we fill in the\n> \"remote_name\" field from the config, and then fill in the\n> actual \"remote\" field based on that name. However, it turns\n> out that nobody really cares about this field. The only two\n> sites that access it are:\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\" for\n> that; we can just look it up for the duration of the\n> operation.\n>\n> Getting rid of it drops one potential source of confusion:\n> is the value the match for \"remote_name\", or is it the\n> remote we would fetch from when on that branch (i.e., does\n> it fall back to \"origin\")?\n\nI had to read the above three times before I realized that you were\nwondering \"what is the value of this 'remote' field? is it what\nremote_get() would give us for 'remote_name' and is NULL if\nremote_name is not set, or is it never NULL and instead have the\nremote for 'origin' if remote_name is not set?\"\n\nBut perhaps it is just me.\n\nWe certainly have duplicated information between the two fields, and\nit first looked somewhat unnatural that you kept the name with which\nyou need to trigger a search for the structure, instead of keeping\nthe structure, one of whose field is its name already. Perhaps\nthere was a valid reason behind this choice, and I am guessing that\nit is probably because it will not let you differenciate the case\nwhere the user explicitly said 'origin' and we used 'origin' as a\nfallback, if you keep a \"remote\" field that stores the instance of\nthe remote structure for 'origin' without keeping \"remote_name\".\n\nBut we shouldn't leave it for readers to guess.\n\n> When we add pushremote_name, this question would get even\n> more confusing, as pushremotes have a more complicated\n> lookup procedure. It would be nice for the code to be\n> consistent between the remote and pushremote, and this takes\n> us one step closer.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/merge.c | 2 +-\n> remote.c | 14 ++++++++------\n> remote.h | 1 -\n> 3 files changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 3b0f8f9..1840317 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -955,7 +955,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.\"));\n> diff --git a/remote.c b/remote.c\n> index fcd868d..d5fd605 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1633,15 +1633,20 @@ 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> \n> +\tif (!ret->remote_name || !ret->merge_nr)\n> +\t\treturn;\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> @@ -1661,11 +1666,8 @@ 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> -\t\tif (ret->merge_nr)\n> -\t\t\tset_merge(ret);\n> -\t}\n> +\tif (ret)\n> +\t set_merge(ret);\n> \treturn ret;\n> }\n> \n> diff --git a/remote.h b/remote.h\n> index 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"},{"id":"258777","messageId":"xmqqk2xwq25m.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331173834.GF18912@peff.net","subject":"Re: [PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T21:37:25Z","receivedAt":"2015-03-31T21:37:25Z","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/Documentation/revisions.txt b/Documentation/revisions.txt\n> index 0796118..5d9df25 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\nThe corresponding description for upstream begins like this:\n\n The suffix '@\\{upstream\\}' to a branchname (short form '@\\{u\\}')\n\nand makes me wonder if the existing backslashes are unnecessary, or\nif you forgot to use them in the new text.\n\n> @@ -1104,6 +1111,95 @@ static int interpret_upstream_mark(const char *name, int namelen,\n> \treturn len + at;\n> }\n> \n> +static char *tracking_ref_for(struct remote *remote, const char *refname)\n> +{\n> +\tchar *ret;\n> +\n> +\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n> +\tif (!ret)\n> +\t\tdie(_(\"@{push} has no local tracking branch for remote '%s'\"),\n> +\t\t refname);\n\nI would imagine that it would be very plausible that anybody with a\nspecific remote and the name of the ref that appears on that remote\nwould want to learn the local name of the remote-tracking ref we use\nto track it.\n\nBut the error message limits the callers only to those who are\ninvolved in @{push} codepath. Shouldn't the error check be done in\nthe caller instead, anticipating the day this useful function ceases\nto be static?\n\nI would suspect that such a change would make it just a one-liner,\nbut I think this helper that takes remote and their refname is much\neasier to read than four inlined calls to apply_refspecs() that have\nto spell out remote->fetch, remote->fetch_refspec_nr separately.\n\nPerhaps we would want \n\n\tstruct refspecs {\n \tint nr, alloc;\n const char **refspec;\n\t} fetch_refspec;\n\nin \"struct remote\", instead of these two separate fields, and then\nmake apply_refspecs() take \"struct refspecs *\"? I haven't checked\nand thought enough to decide if we want \"struct refspec *\" also in\nthat new struct, though.\n"},{"id":"258779","messageId":"CAPig+cR9U9Tm1Lbk_wQpfvY7okEKvYgMvNRzT1ci2u9ALr0HpQ@mail.gmail.com","threadId":"38961","inReplyTo":"20150331173834.GF18912@peff.net","subject":"Re: [PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-31T21:41:02Z","receivedAt":"2015-03-31T21:41:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 31, 2015 at 1:38 PM, Jeff King wrote:\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> index 0796118..5d9df25 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` is no branchname is specified). Since our push destination is\n\ns/is no/if no/\n\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.\n"},{"id":"258778","messageId":"xmqqfv8kq1z8.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331173727.GD18912@peff.net","subject":"Re: [PATCH 4/6] remote.c: provide per-branch pushremote name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T21:41:15Z","receivedAt":"2015-03-31T21:41:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Let's make this more like the fetch-remote config. We'll\n> record the pushremote for each branch, and then explicitly\n> compute the correct remote for the current branch at the\n> time of reading.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote.c | 41 +++++++++++++++++++++++------------------\n> remote.h | 2 ++\n> 2 files changed, 25 insertions(+), 18 deletions(-)\n\nVery nicely done. Thanks.\n"},{"id":"258784","messageId":"20150331222224.GA31948@peff.net","threadId":"38961","inReplyTo":"xmqqzj6sq4xc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/6] remote.c: drop default_remote_name variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T22:22:25Z","receivedAt":"2015-03-31T22:22:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 31, 2015 at 01:37:35PM -0700, Junio C Hamano wrote:\n\n> > When we read the remote config from disk, we update a\n> > default_remote_name variable if we see branch.*.remote\n> > config for the current branch. This isn't wrong, or even all\n> > that complicated, but it is a bit simpler (because it\n> > reduces our overall state) to just lazily compute the\n> > default when we need it.\n> >\n> > The ulterior motive here is that the push config uses a\n> > similar structure, and _is_ much more complicated as a\n> > result. That will be simplified in a future patch, and it's\n> > more readable if the logic remotes and push-remotes matches.\n> \n> I cannot quite parse the part after \"if...\"; \"the logic used by\n> remotes and push-remotes matches\"?\n\nSorry, it should have been \"...if the logic for remotes and push-remotes\nmatches\".\n\n> > +static int loaded_remotes_config;\n> > +\n> \n> I expect that the reason why this is not a function scope static in\n> read_config() will be revealed in the later patch in this series...\n\nNo, it's not used. I was leaving it as an \"out\" in case somebody ever\nneeded to reset it in order to re-read the config, but I agree it is\nprobably better to let them pull it out of the function if and when that\nhappens.\n\n-Peff\n"},{"id":"258785","messageId":"20150331222431.GB31948@peff.net","threadId":"38961","inReplyTo":"xmqqvbhgq4ci.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/6] remote.c: drop \"remote\" pointer from \"struct branch\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T22:24:31Z","receivedAt":"2015-03-31T22:24:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 31, 2015 at 01:50:05PM -0700, Junio C Hamano wrote:\n\n> > Getting rid of it drops one potential source of confusion:\n> > is the value the match for \"remote_name\", or is it the\n> > remote we would fetch from when on that branch (i.e., does\n> > it fall back to \"origin\")?\n> \n> I had to read the above three times before I realized that you were\n> wondering \"what is the value of this 'remote' field? is it what\n> remote_get() would give us for 'remote_name' and is NULL if\n> remote_name is not set, or is it never NULL and instead have the\n> remote for 'origin' if remote_name is not set?\"\n> \n> But perhaps it is just me.\n> \n> We certainly have duplicated information between the two fields, and\n> it first looked somewhat unnatural that you kept the name with which\n> you need to trigger a search for the structure, instead of keeping\n> the structure, one of whose field is its name already. Perhaps\n> there was a valid reason behind this choice, and I am guessing that\n> it is probably because it will not let you differenciate the case\n> where the user explicitly said 'origin' and we used 'origin' as a\n> fallback, if you keep a \"remote\" field that stores the instance of\n> the remote structure for 'origin' without keeping \"remote_name\".\n\nThat is the reason I was trying to explain above. Though I suppose you\ncould argue that remote_name suffers the same question (i.e., would we\never set it to \"origin\"?)\n\nIt is much worse for pushremotes, which can come from\nbranch.*.pushremote, remote.pushdefault, branch.*.remote, or \"origin\".\n\nI'll try to re-word the commit message.\n\n-Peff\n"},{"id":"258787","messageId":"xmqq384kpzqb.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331222431.GB31948@peff.net","subject":"Re: [PATCH 2/6] remote.c: drop \"remote\" pointer from \"struct branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T22:29:48Z","receivedAt":"2015-03-31T22:29:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 31, 2015 at 01:50:05PM -0700, Junio C Hamano wrote:\n>\n>> it first looked somewhat unnatural that you kept the name with which\n>> you need to trigger a search for the structure, instead of keeping\n>> the structure, one of whose field is its name already.\n> ...\n> That is the reason I was trying to explain above. Though I suppose you\n> could argue that remote_name suffers the same question (i.e., would we\n> ever set it to \"origin\"?)\n\nWell, another would be that by keeping remote_name and making remote\non-demand, we may still have to keep all the defined branches in\ncore but we do not have to instanciate all the remotes, if each\nbranch only knows the remote_name. A single look-up may be cheap\nbut that is not a good reason to do one-per-each-branch if we do not\nneed to.\n\n> It is much worse for pushremotes, which can come from\n> branch.*.pushremote, remote.pushdefault, branch.*.remote, or \"origin\".\n>\n> I'll try to re-word the commit message.\n\nThanks.\n"},{"id":"258790","messageId":"20150331223200.GC31948@peff.net","threadId":"38961","inReplyTo":"xmqqk2xwq25m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T22:32:01Z","receivedAt":"2015-03-31T22:32:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 31, 2015 at 02:37:25PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> > index 0796118..5d9df25 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> \n> The corresponding description for upstream begins like this:\n> \n> The suffix '@\\{upstream\\}' to a branchname (short form '@\\{u\\}')\n> \n> and makes me wonder if the existing backslashes are unnecessary, or\n> if you forgot to use them in the new text.\n\nThey are necessary inside single-quotes, but not inside backticks. IMHO\nthis entire file should be using backticks, but I didn't want to\nreformat the entire file (and so I tried to at least keep the heading in\nthe same style as the rest of it).\n\n> > +static char *tracking_ref_for(struct remote *remote, const char *refname)\n> > +{\n> > +\tchar *ret;\n> > +\n> > +\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n> > +\tif (!ret)\n> > +\t\tdie(_(\"@{push} has no local tracking branch for remote '%s'\"),\n> > +\t\t refname);\n> \n> I would imagine that it would be very plausible that anybody with a\n> specific remote and the name of the ref that appears on that remote\n> would want to learn the local name of the remote-tracking ref we use\n> to track it.\n\nI am not sure I understand. We do _not_ have a local name we use to\ntrack it. That is the error. I can print \"remote %s does not have branch\n%s\", if that is what you mean.\n\n> But the error message limits the callers only to those who are\n> involved in @{push} codepath. Shouldn't the error check be done in\n> the caller instead, anticipating the day this useful function ceases\n> to be static?\n\nIs it really a useful general function? If you remove the die() message,\nit is literally a one-liner. My purpose in pulling it out at all was not\nto repeat the die() message over and over in get_push_branch().\n\n> I would suspect that such a change would make it just a one-liner,\n> but I think this helper that takes remote and their refname is much\n> easier to read than four inlined calls to apply_refspecs() that have\n> to spell out remote->fetch, remote->fetch_refspec_nr separately.\n> \n> Perhaps we would want \n> \n> \tstruct refspecs {\n> \tint nr, alloc;\n> const char **refspec;\n> \t} fetch_refspec;\n> \n> in \"struct remote\", instead of these two separate fields, and then\n> make apply_refspecs() take \"struct refspecs *\"? I haven't checked\n> and thought enough to decide if we want \"struct refspec *\" also in\n> that new struct, though.\n\nI think it is more complicated, as there are actually two arrays indexed\nby each {fetch,push}_refspec_nr. We have \"fetch_respec\", which contains\nthe text (I assume), and then the \"struct refspec\". So ideally those\nwould be stored together in a single list, but of course many helper\nfunctions want just the \"struct refspec\" list. So you still end up with\ntwo lists, but just pushed down into a single struct. I guess that's\nbetter, but I was trying to find a bound to my refactoring rather than\ntouching all of the code. :-/\n\n-Peff\n"},{"id":"258791","messageId":"20150331223332.GD31948@peff.net","threadId":"38961","inReplyTo":"CAPig+cR9U9Tm1Lbk_wQpfvY7okEKvYgMvNRzT1ci2u9ALr0HpQ@mail.gmail.com","subject":"Re: [PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T22:33:33Z","receivedAt":"2015-03-31T22:33:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 31, 2015 at 05:41:02PM -0400, Eric Sunshine wrote:\n\n> On Tue, Mar 31, 2015 at 1:38 PM, Jeff King wrote:\n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> > index 0796118..5d9df25 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` is no branchname is specified). Since our push destination is\n> \n> s/is no/if no/\n\nThanks, fixed for my re-roll.\n\n-Peff\n"},{"id":"258794","messageId":"xmqqmw2sojv2.fsf@gitster.dls.corp.google.com","threadId":"38961","inReplyTo":"20150331223200.GC31948@peff.net","subject":"Re: [PATCH 6/6] sha1_name: implement @{push} shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T22:57:53Z","receivedAt":"2015-03-31T22:57:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > +static char *tracking_ref_for(struct remote *remote, const char *refname)\n>> > +{\n>> > +\tchar *ret;\n>> > +\n>> > +\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n>> > +\tif (!ret)\n>> > +\t\tdie(_(\"@{push} has no local tracking branch for remote '%s'\"),\n>> > +\t\t refname);\n>> \n>> I would imagine that it would be very plausible that anybody with a\n>> specific remote and the name of the ref that appears on that remote\n>> would want to learn the local name of the remote-tracking ref we use\n>> to track it.\n>\n> I am not sure I understand. We do _not_ have a local name we use to\n> track it. That is the error. I can print \"remote %s does not have branch\n> %s\", if that is what you mean.\n\nNo, I am not saying we should not detect an error.\n\n>> But the error message limits the callers only to those who are\n>> involved in @{push} codepath. Shouldn't the error check be done in\n>> the caller instead, anticipating the day this useful function ceases\n>> to be static?\n>\n> Is it really a useful general function?\n\nI often interact with this remote called 'ko'. Do I have remote-tracking\nbranch to keep track of its 'master' branch, and if so, what is it?\n\nIsn't that the question that the apply_refspecs() is answering?\n\n> If you remove the die() message,\n> it is literally a one-liner.\n\nYes, I thought I said that here...\n\n>> I would suspect that such a change would make it just a one-liner,\n>> but I think this helper that takes remote and their refname is much\n>> easier to read than four inlined calls to apply_refspecs() that have\n>> to spell out remote->fetch, remote->fetch_refspec_nr separately.\n\n>> Perhaps we would want \n>> \n>> \tstruct refspecs {\n>> \tint nr, alloc;\n>> const char **refspec;\n>> \t} fetch_refspec;\n>> \n>> in \"struct remote\", instead of these two separate fields, and then\n>> make apply_refspecs() take \"struct refspecs *\"? I haven't checked\n>> and thought enough to decide if we want \"struct refspec *\" also in\n>> that new struct, though.\n>\n> I think it is more complicated, as there are actually two arrays indexed\n> by each {fetch,push}_refspec_nr. We have \"fetch_respec\", which contains\n> the text (I assume), and then the \"struct refspec\".\n\nYeah, I thought I said that, too ;-)\n\n> So ideally those\n> would be stored together in a single list, but of course many helper\n> functions want just the \"struct refspec\" list. So you still end up with\n> two lists, but just pushed down into a single struct. I guess that's\n> better, but I was trying to find a bound to my refactoring rather than\n> touching all of the code. :-/\n\nOK.\n"}]}