{"thread":{"id":"53171","subject":"[PATCH v6 1/2] remote.c: fix %(push) for triangular workflows","startedAt":"2020-04-06T17:57:23Z","lastAt":"2020-04-06T17:57:29Z","messageCount":3,"participants":["Damien Robert"],"isPatch":true,"patchVersion":6,"patchTotal":2},"messages":[{"id":"394867","messageId":"20200406175648.25737-2-damien.olivier.robert+git@gmail.com","threadId":"53171","inReplyTo":"20200406175648.25737-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v6 1/2] remote.c: fix %(push) for triangular workflows","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-06T17:56:47Z","receivedAt":"2020-04-06T17:57:23Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"The behaviour of `git push` when push.default is simple or upstream\nchanges in a triangular workflow, but this was not taken into account by\n%(push). Update the code to detect triangular workflows and fix this.\n\nSigned-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>\n---\n remote.c | 42 +++++++++++++++++++++++++++++++-----------\n 1 file changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c43196ec06..3750a2bcc1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1656,6 +1656,18 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n+static int is_workflow_triangular(struct branch *branch)\n+{\n+\tstruct remote *fetch_remote = remote_get(remote_for_branch(branch, NULL));\n+\tstruct remote *push_remote = remote_get(pushremote_for_branch(branch, NULL));\n+\treturn (fetch_remote && push_remote && fetch_remote != push_remote);\n+}\n+\n+/**\n+ * Return the tracking branch, as in %(push), that corresponds to the ref we\n+ * would push to given a bare `git push` while `branch` is checked out.\n+ * See also branch_get_push_remoteref above.\n+ */\n static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n {\n \tstruct remote *remote;\n@@ -1693,23 +1705,31 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\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+\t\tif (is_workflow_triangular(branch))\n+\t\t\treturn error_buf(err, _(\"push has no destination (push.default is 'upstream' and we are in a triangular workflow)\"));\n+\t\telse\n+\t\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\tif (is_workflow_triangular(branch)) {\n+\t\t\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\tup = branch_get_upstream(branch, err);\n+\t\t\t\tif (!up)\n+\t\t\t\t\treturn NULL;\n+\t\t\t\tcur = tracking_for_push_dest(remote, branch->refname, err);\n+\t\t\t\tif (!cur)\n+\t\t\t\t\treturn NULL;\n+\t\t\t\tif (strcmp(cur, up))\n+\t\t\t\t\treturn error_buf(err,\n+\t\t\t\t\t\t\t _(\"cannot resolve 'simple' push to a single destination\"));\n+\t\t\t\treturn cur;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \nPatched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)\n\n"},{"id":"394868","messageId":"20200406175648.25737-3-damien.olivier.robert+git@gmail.com","threadId":"53171","inReplyTo":"20200406175648.25737-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v6 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-06T17:56:48Z","receivedAt":"2020-04-06T17:57:25Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"Looking at the value of %(push:remoteref) only handles the case when an\nexplicit push refspec is passed. But it does not handle the fallback\ncases of looking at the configuration value of `push.default`.\n\nIn particular, doing something like\n\n    git config push.default current\n    git for-each-ref --format='%(push)'\n    git for-each-ref --format='%(push:remoteref)'\n\nprints a useful tracking ref for the first for-each-ref, but an empty\nstring for the second.\n\nSince the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:\nlet upstream/push report the remote ref name) is to get exactly which\nbranch `git push` will push to, even in the fallback cases, fix this.\n\nTo get the meaning of %(push:remoteref), `ref-filter.c` calls\n`remote_ref_for_branch`. We simply add a new static helper function,\n`branch_get_push_remoteref` that follows the logic of\n`branch_get_push_1`, and call it from `remote_ref_for_branch`.\n\nWe also update t/6300-for-each-ref.sh to handle all `push.default`\nstrategies. This involves testing `push.default=simple` twice, once\nwhere there is a matching upstream branch and once when there is none.\n\nFinally we also test for triangular workflows.\n\nSigned-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>\n---\n remote.c                | 97 +++++++++++++++++++++++++++++++----------\n t/t6300-for-each-ref.sh | 81 +++++++++++++++++++++++++++++++---\n 2 files changed, 149 insertions(+), 29 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3750a2bcc1..2b7f8a3af5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)\n \treturn remote_for_branch(branch, explicit);\n }\n \n-const char *remote_ref_for_branch(struct branch *branch, int for_push)\n-{\n-\tif (branch) {\n-\t\tif (!for_push) {\n-\t\t\tif (branch->merge_nr) {\n-\t\t\t\treturn branch->merge_name[0];\n-\t\t\t}\n-\t\t} else {\n-\t\t\tconst char *dst, *remote_name =\n-\t\t\t\tpushremote_for_branch(branch, NULL);\n-\t\t\tstruct remote *remote = remote_get(remote_name);\n-\n-\t\t\tif (remote && remote->push.nr &&\n-\t\t\t    (dst = apply_refspecs(&remote->push,\n-\t\t\t\t\t\t  branch->refname))) {\n-\t\t\t\treturn dst;\n-\t\t\t}\n-\t\t}\n-\t}\n-\treturn NULL;\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@@ -1663,6 +1641,67 @@ static int is_workflow_triangular(struct branch *branch)\n \treturn (fetch_remote && push_remote && fetch_remote != push_remote);\n }\n \n+/**\n+ * Return the local name of the remote tracking branch, as in\n+ * %(push:remoteref), that corresponds to the ref we would push to given a\n+ * bare `git push` while `branch` is checked out.\n+ * See also branch_get_push_1 below.\n+ */\n+static const char *branch_get_push_remoteref(struct branch *branch)\n+{\n+\tstruct remote *remote;\n+\n+\tremote = remote_get(pushremote_for_branch(branch, NULL));\n+\tif (!remote)\n+\t\treturn NULL;\n+\n+\tif (remote->push.nr) {\n+\t\treturn apply_refspecs(&remote->push, branch->refname);\n+\t}\n+\n+\tif (remote->mirror)\n+\t\treturn branch->refname;\n+\n+\tswitch (push_default) {\n+\tcase PUSH_DEFAULT_NOTHING:\n+\t\treturn NULL;\n+\n+\tcase PUSH_DEFAULT_MATCHING:\n+\tcase PUSH_DEFAULT_CURRENT:\n+\t\treturn branch->refname;\n+\n+\tcase PUSH_DEFAULT_UPSTREAM:\n+\t\tif (is_workflow_triangular(branch))\n+\t\t    return NULL;\n+\t\telse {\n+\t\t\tif (branch && branch->merge && branch->merge[0] &&\n+\t\t    \t    branch->merge[0]->dst)\n+\t\t\t\treturn branch->merge[0]->src;\n+\t\t\telse\n+\t\t\t\treturn NULL;\n+\t\t}\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\tif (is_workflow_triangular(branch))\n+\t\t\t\treturn branch->refname;\n+\t\t\telse {\n+\t\t\t\tup = branch_get_upstream(branch, NULL);\n+\t\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n+\t\t\t\tif (up && cur && !strcmp(cur, up))\n+\t\t\t\t\treturn branch->refname;\n+\t\t\t\telse\n+\t\t\t\t\treturn NULL;\n+\t\t\t}\n+\n+\t\t}\n+\t}\n+\tBUG(\"unhandled push situation\");\n+}\n+\n /**\n  * Return the tracking branch, as in %(push), that corresponds to the ref we\n  * would push to given a bare `git push` while `branch` is checked out.\n@@ -1755,6 +1794,20 @@ static int ignore_symref_update(const char *refname)\n \treturn (flag & REF_ISSYMREF);\n }\n \n+const char *remote_ref_for_branch(struct branch *branch, int for_push)\n+{\n+\tif (branch) {\n+\t\tif (!for_push) {\n+\t\t\tif (branch->merge_nr) {\n+\t\t\t\treturn branch->merge_name[0];\n+\t\t\t}\n+\t\t} else {\n+\t\t\treturn branch_get_push_remoteref(branch);\n+\t\t}\n+\t}\n+\treturn NULL;\n+}\n+\n /*\n  * Create and return a list of (struct ref) consisting of copies of\n  * each remote_ref that matches refspec.  refspec must be a pattern.\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex b3c1092338..8e59ab2567 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -875,13 +875,80 @@ test_expect_success ':remotename and :remoteref' '\n \t\t\tgit for-each-ref --format=\"${pair%=*}\" \\\n \t\t\t\trefs/heads/master >actual &&\n \t\t\ttest_cmp expect actual\n-\t\tdone &&\n-\t\tgit branch push-simple &&\n-\t\tgit config branch.push-simple.pushRemote from &&\n-\t\tactual=\"$(git for-each-ref \\\n-\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n-\t\t\trefs/heads/push-simple)\" &&\n-\t\ttest from, = \"$actual\"\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success '%(push) and %(push:remoteref)' '\n+\tgit init pushremote-tests &&\n+\t(\n+\t\tcd pushremote-tests &&\n+\t\ttest_commit initial &&\n+\t\tgit remote add from fifth.coffee:blub &&\n+\t\tgit config branch.master.remote from &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,, = \"$actual\" &&\n+\t\tgit config branch.master.merge refs/heads/master &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/master,refs/remotes/from/master = \"$actual\" &&\n+\t\tgit config branch.master.merge refs/heads/other &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,, = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=upstream for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/other,refs/remotes/from/other = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/master,refs/remotes/from/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/master,refs/remotes/from/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,, = \"$actual\" &&\n+\t\tgit remote add to southridge.audio:repo &&\n+\t\tgit config branch.master.pushRemote to &&\n+\t\tgit config --unset branch.master.merge &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,refs/heads/master,refs/remotes/to/master = \"$actual\" &&\n+\t\tgit config branch.master.merge refs/heads/master &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,refs/heads/master,refs/remotes/to/master = \"$actual\" &&\n+\t\tgit config branch.master.merge refs/heads/other &&\n+\t\tactual=\"$(git -c push.default=simple for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,refs/heads/master,refs/remotes/to/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=upstream for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,, = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,refs/heads/master,refs/remotes/to/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,refs/heads/master,refs/remotes/to/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref),%(push)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest to,, = \"$actual\"\n \t)\n '\n \n-- \nPatched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)\n\n"},{"id":"394869","messageId":"20200406175648.25737-1-damien.olivier.robert+git@gmail.com","threadId":"53171","inReplyTo":"20200312164558.2388589-1-damien.olivier.robert+git@gmail.com","subject":"[RFC PATCH v4 0/2] %(push) and %(push:remoteref) bug fixes","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-06T17:56:46Z","receivedAt":"2020-04-06T17:57:29Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"This fix several bugs in for-each-ref for %(push) and %(push:remoteref), as\nexplained in the commit messages.\n\nNote that there are still several bugs:\n- the memory leak mentioned by Jeff in\n  https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/\n\n- in my patch, to detect if the workflow is triangular, I use:\n\nstatic int is_workflow_triangular(struct branch *branch)\n{\n\tstruct remote *fetch_remote = remote_get(remote_for_branch(branch, NULL));\n\tstruct remote *push_remote = remote_get(pushremote_for_branch(branch, NULL));\n\treturn (fetch_remote && push_remote && fetch_remote != push_remote);\n}\n\nBut remote_get will always fallback to 'origin'. So this means that if we\nset up a pushRemote=\"foobar\" and no 'remote', the workflow is detected as\ntriangular.\n\nWhereas in `git push`, this workflow will not be detected as triangular.\n\n=> So I can check that by looking at *explicit, but I actually have a\nquestion about what constitutes a triangular workflow, hence the RFC.\n\nFurthermore, the upstream (and simple in non triangular workflow) case of\n%(push) and (push:remoteref) are essentially via `branch_get_upstream`, which\nis also used for %(upstream):\n\n\tbranch && branch->merge && branch->merge[0] &&\n\t\t    \t    branch->merge[0]->dst)\n\nbut `git push` does different checks:\n\n\tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"...\n\tif (branch->merge_nr != 1)\n\t\tdie(_(\"The current branch %s has multiple upstream branches, \"\n\t\t    \"refusing to push.\"), branch->name);\n\nin particular git push fails if merge_nr !=1 or if branch has no remote,\nwhereas %(push) will still indicates a push branch (assuming I fix\nis_workflow_triangular).\n\nSo I'll need to add a `branch_get_push` with these checks instead.\n\nSo I first send this patch as an RFC, and I'll see how to proceed\nafterwards to handle these remaining corner cases.\nLuckily, having a pushRemote but no remote, or several merge in the branch\nconfig are probably not too common.\n\n=> So one question I have first is about the case when we do have a\nbranch.pushRemote but not a branch.remote.\nShould this still be considered a triangular workflow?\n\nAccording to git-push, no:\n\n\tstatic int is_workflow_triangular(struct remote *remote)\n\t{\n\t\tstruct remote *fetch_remote = remote_get(NULL);\n\t\treturn (fetch_remote && fetch_remote != remote);\n\t}\n\nbut I would argue that we should.\n\nThis would change nothing for push.default=upstream, since currently we\ncheck that `branch` has a remote_name in `setup_push_upstream` so it fails\nanyway even if the workflow is not explicitly triangular, but this would\nmake push.default=simple behave as current, exactly as when branch.remote\nis different from branch.pushRemote (and I would argue that no\nbranch.remote is a particular case of this situation).\n\n\nPS: the first patch has no tests because I add them in the second patch, it\nis more convenient to add them at once and test both patches.\n\nPPS: v4 and v5 are intermediate versions I made but did not send to the ML.\n\n\nDamien Robert (2):\n  remote.c: fix %(push) for triangular workflows\n  remote.c: fix handling of %(push:remoteref)\n\n remote.c                | 139 ++++++++++++++++++++++++++++++----------\n t/t6300-for-each-ref.sh |  81 +++++++++++++++++++++--\n 2 files changed, 180 insertions(+), 40 deletions(-)\n\n-- \nPatched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)\n\n"}]}