{"thread":{"id":"52910","subject":"[PATCH 1/1] remote.c: fix handling of push:remote_ref","startedAt":"2020-02-28T17:25:30Z","lastAt":"2020-09-14T22:21:33Z","messageCount":32,"participants":["Damien Robert","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"392675","messageId":"20200228172455.1734888-1-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":null,"subject":"[PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T17:24:55Z","receivedAt":"2020-02-28T17:25:30Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"To get the meaning of push:remoteref, ref-filter.c calls\nremote_ref_for_branch.\n\nHowever remote_ref_for_branch only handles the case of a specified refspec.\nThe other cases treated by branch_get_push_1 are the mirror case,\nPUSH_DEFAULT_{NOTHING,MATCHING,CURRENT,UPSTREAM,UNSPECIFIED,SIMPLE}.\n\nIn all these cases, either there is no push remote, or the remote_ref is\nbranch->refname. So we can handle all these cases by returning\nbranch->refname, provided that remote is not empty.\n\nSigned-off-by: Damien Robert \n---\n remote.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 593ce297ed..75e42b1e36 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\n \t\t\t\t\t*explicit = 1;\n \t\t\t\treturn dst;\n \t\t\t}\n+\t\t\telse if (remote) {\n+\t\t\t\tif (explicit)\n+\t\t\t\t\t*explicit = 1;\n+\t\t\t\treturn branch->refname;\n+\t\t\t}\n \t\t}\n \t}\n \tif (explicit)\n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392678","messageId":"20200228182349.GA1408759@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200228172455.1734888-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-28T18:23:49Z","receivedAt":"2020-02-28T18:23:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2020 at 06:24:55PM +0100, Damien Robert wrote:\n\n> To get the meaning of push:remoteref, ref-filter.c calls\n> remote_ref_for_branch.\n> \n> However remote_ref_for_branch only handles the case of a specified refspec.\n> The other cases treated by branch_get_push_1 are the mirror case,\n> PUSH_DEFAULT_{NOTHING,MATCHING,CURRENT,UPSTREAM,UNSPECIFIED,SIMPLE}.\n\nJust to back up a minute to the user-visible problem, it's that:\n\n git config push.default matching\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. That feature (and remote_ref_for_branch) come\nfrom 9700fae5ee (for-each-ref: let upstream/push report the remote ref\nname, 2017-11-07). Author cc'd for guidance.\n\nI wonder if %(upstream:remoteref) has similar problems, but I suppose\nnot (it doesn't have this implicit config, so we'd always either have a\nremote ref or we'd have no upstream at all).\n\n> In all these cases, either there is no push remote, or the remote_ref is\n> branch->refname. So we can handle all these cases by returning\n> branch->refname, provided that remote is not empty.\n\nIn the case of \"upstream\", the names could be different, couldn't they?\n\nIf I do this:\n\n git init parent\n git -C parent commit --allow-empty -m foo\n \n git clone parent child\n cd child\n git branch --track mybranch origin/master\n git config push.default upstream\n git for-each-ref \\\n --format='push=%(push), remoteref=%(push:remoteref)' \\\n refs/heads/mybranch\n\nthe current code gives no remoteref value, which seems wrong. But with\nyour patch I'd get \"refs/heads/mybranch\", which is also wrong.\n\nI think you're right that all of the other cases would always use the\nsame refname on the remote.\n\n> remote.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n\nWe'd want some test coverage to make sure this doesn't regress. There\nare already some tests covering this feature in t6300. And indeed, your\npatch causes them to fail when checking a \"simple\" push case (but I\nthink I'd argue the current expected value there is wrong). That should\nbe expanded to cover the \"upstream\" case, too, once we figure out how to\nget it right.\n\n> diff --git a/remote.c b/remote.c\n> index 593ce297ed..75e42b1e36 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\n> \t\t\t\t\t*explicit = 1;\n> \t\t\t\treturn dst;\n> \t\t\t}\n> +\t\t\telse if (remote) {\n> +\t\t\t\tif (explicit)\n> +\t\t\t\t\t*explicit = 1;\n> +\t\t\t\treturn branch->refname;\n> +\t\t\t}\n\nSaying \"*explicit = 1\" here seems weird. Isn't the whole point that\nthese modes _aren't_ explicit?\n\nIt looks like our only caller will ignore our return value unless we say\n\"explicit\", though. I have to wonder what the point of that flag is,\nversus just returning NULL when we don't have anything to return.\n\n-Peff\n"},{"id":"392732","messageId":"20200301220531.iuokzzdb5gruslrn@doriath","threadId":"52910","inReplyTo":"20200228182349.GA1408759@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-01T22:05:31Z","receivedAt":"2020-03-01T22:05:40Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"First I apologize for sending the patch too soon, I forgot to put the RFC\nflag, this is not a complete patch, and I do intend to fix the tests. I was\naware of this issue since my patch about push:track, but since I don't use\npush:remoteref this was a low priority to fix. Last friday I was sick and\ncould not work, so I took the opportunity to scratch this itch.\n\nFrom Jeff King, Fri 28 Feb 2020 at 13:23:49 (-0500) :\n> Just to back up a minute to the user-visible problem, it's that:\n> git config push.default matching\n> git for-each-ref --format='%(push)'\n> git for-each-ref --format='%(push:remoteref)'\n> prints a useful tracking ref for the first for-each-ref, but an empty\n> string for the second. That feature (and remote_ref_for_branch) come\n> from 9700fae5ee (for-each-ref: let upstream/push report the remote ref\n> name, 2017-11-07). Author cc'd for guidance.\n\nYes exactly.\n \n> I wonder if %(upstream:remoteref) has similar problems, but I suppose\n> not (it doesn't have this implicit config, so we'd always either have a\n> remote ref or we'd have no upstream at all).\n\nAnd the code is different. upstream:remoteref uses branch->merge_name[0].\nThis is due to the fact that the branch struct stores different things for\nmerge branches than for push branches (which hurt my sense of symmetry :)).\n\n> > In all these cases, either there is no push remote, or the remote_ref is\n> > branch->refname. So we can handle all these cases by returning\n> > branch->refname, provided that remote is not empty.\n> In the case of \"upstream\", the names could be different, couldn't they?\n\nYes of course. Moreover there is also the case of 'nothing' where we should\nnot return the branch name (so what we should test for in the other cases\nis not the existence of `remote` but of `branch->push_remote_ref`.)\n\n> We'd want some test coverage to make sure this doesn't regress. There\n> are already some tests covering this feature in t6300. And indeed, your\n> patch causes them to fail when checking a \"simple\" push case (but I\n> think I'd argue the current expected value there is wrong). That should\n> be expanded to cover the \"upstream\" case, too, once we figure out how to\n> get it right.\n\nYes, I'll do both simple and upstream for tests I think.\n\n> > diff --git a/remote.c b/remote.c\n> > index 593ce297ed..75e42b1e36 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\n> > \t\t\t\t\t*explicit = 1;\n> > \t\t\t\treturn dst;\n> > \t\t\t}\n> > +\t\t\telse if (remote) {\n> > +\t\t\t\tif (explicit)\n> > +\t\t\t\t\t*explicit = 1;\n> > +\t\t\t\treturn branch->refname;\n> > +\t\t\t}\n> \n> Saying \"*explicit = 1\" here seems weird. Isn't the whole point that\n> these modes _aren't_ explicit?\n\nWell pushremote_for_branch also set explicit=1 if only remote.pushDefault\nis set, so I followed suit.\n\n> It looks like our only caller will ignore our return value unless we say\n> \"explicit\", though. I have to wonder what the point of that flag is,\n> versus just returning NULL when we don't have anything to return.\n\nI think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the\nsituation is handled by RR_REMOTE_REF, where explicit is not used at all.\nSo we could remove it.\n\n\nSo it remains the problem of handling the 'upstream' case.\nThe ideal solution would be to not duplicate branch_get_push_1.\nIn most of the case, this function finds `dst` which is exactly the\npush:remoteref we are looking for. \n\nThen branch_get_push_1 uses\n\t\tret = tracking_for_push_dest(remote, dst, err);\nwhich simply calls\n\tret = apply_refspecs(&remote->fetch, dst);\n\nThe only different case is\n\tcase PUSH_DEFAULT_UPSTREAM:\n\t\treturn branch_get_upstream(branch, err);\nwhich returns\n\tbranch->merge[0]->dst\n\nSo we could almost write an auxiliary function that returns push:remoteref\nand use it both in remote_ref_for_branch and branch_get_push_1 (via a\nfurther call to tracking_for_push_dest) except for the 'upstream' case\nwhich is subtly different.\n\nIn the 'upstream' case, the auxiliary function would return\nbranch->merge_name[0]. So the question is: can\ntracking_for_push_dest(branch->merge_name[0]) be different from\nbranch->merge[0]->dst?\n\nNow branch->merge is set in `set_merge`, where it is constructed via\n\t\tif (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),\n\t\t\t &oid, &ref) == 1)\nAnd I don't understand dwim_ref enough to know if there could be\ndifferences in our setting from tracking_for_push_dest in corner cases.\n\n\nAnother solution could be as follow: we already store `push` in\n`branch->push_tracking_ref`. So the question is: can we always easily convert\nsomething like refs/remotes/origin/branch_name to refs/heads/branch_name\n(ie essentially reverse ètracking_for_push_dest`), or are there corner cases?\n\n\nOtherwise a simple but not elegant solution would be to copy paste the\ncode of branch_get_push_1 to remote_ref_for_branch, simply removing the\ncalls to `tracking_for_push_dest` and using remote->branch_name[0] rather\nthan remote->branch[0]->dst for the upstream case.\n\n\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"392739","messageId":"20200302133217.GA1176622@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200301220531.iuokzzdb5gruslrn@doriath","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-02T13:32:17Z","receivedAt":"2020-03-02T13:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[dropping J from cc, since my earlier email bounced]\n\nOn Sun, Mar 01, 2020 at 11:05:31PM +0100, Damien Robert wrote:\n\n> > Saying \"*explicit = 1\" here seems weird. Isn't the whole point that\n> > these modes _aren't_ explicit?\n> \n> Well pushremote_for_branch also set explicit=1 if only remote.pushDefault\n> is set, so I followed suit.\n\nYeah, I think the useless \"explicit\" was a mistake back when the\nfunction was added. See the patch below.\n\n> > It looks like our only caller will ignore our return value unless we say\n> > \"explicit\", though. I have to wonder what the point of that flag is,\n> > versus just returning NULL when we don't have anything to return.\n> \n> I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the\n> situation is handled by RR_REMOTE_REF, where explicit is not used at all.\n> So we could remove it.\n\nWe do look at it, but it's pointless to do so:\n\n $ git grep -hn -C4 remote_ref_for_branch origin:ref-filter.c\n 1461-\t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n 1462-\t\tint explicit;\n 1463-\t\tconst char *merge;\n 1464-\n 1465:\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n 1466-\t\t\t\t\t &explicit);\n 1467-\t\t*s = xstrdup(explicit ? merge : \"\");\n 1468-\t} else\n 1469-\t\tBUG(\"unhandled RR_* enum\");\n\nI think we probably ought to do this as a preparatory patch in your\nseries.\n\n-- >8 --\nSubject: remote: drop \"explicit\" parameter from remote_ref_for_branch()\n\nCommit 9700fae5ee (for-each-ref: let upstream/push report the remote ref\nname, 2017-11-07) added a remote_ref_for_branch() helper, which is\nmodeled after remote_for_branch(). This includes providing an \"explicit\"\nout-parameter that tells the caller whether the remote was configured by\nthe user, or whether we picked a default name like \"origin\".\n\nBut unlike remote names, there's no default case for the remote branch\nname. In any case where we don't set \"explicit\", we'd just an empty\nstring anyway. Let's instead return NULL in this case, letting us\nsimplify the function interface.\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 6 ++----\n remote.c | 11 ++---------\n remote.h | 3 +--\n 3 files changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6867e33648..9837700732 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1459,12 +1459,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\tremote_for_branch(branch, &explicit);\n \t\t*s = xstrdup(explicit ? remote : \"\");\n \t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n-\t\tint explicit;\n \t\tconst char *merge;\n \n-\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n-\t\t\t\t\t &explicit);\n-\t\t*s = xstrdup(explicit ? merge : \"\");\n+\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push);\n+\t\t*s = xstrdup(merge ? merge : \"\");\n \t} else\n \t\tBUG(\"unhandled RR_* enum\");\n }\ndiff --git a/remote.c b/remote.c\nindex 593ce297ed..c43196ec06 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -516,14 +516,11 @@ 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-\t\t\t\t int *explicit)\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\tif (explicit)\n-\t\t\t\t\t*explicit = 1;\n \t\t\t\treturn branch->merge_name[0];\n \t\t\t}\n \t\t} else {\n@@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\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\tif (explicit)\n-\t\t\t\t\t*explicit = 1;\n \t\t\t\treturn dst;\n \t\t\t}\n \t\t}\n \t}\n-\tif (explicit)\n-\t\t*explicit = 0;\n-\treturn \"\";\n+\treturn NULL;\n }\n \n static struct remote *remote_get_1(const char *name,\ndiff --git a/remote.h b/remote.h\nindex b134cc21be..11d8719b58 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -261,8 +261,7 @@ struct branch {\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-const char *remote_ref_for_branch(struct branch *branch, int for_push,\n-\t\t\t\t int *explicit);\n+const char *remote_ref_for_branch(struct branch *branch, int for_push);\n \n /* returns true if the given branch has merge configuration given. */\n int branch_has_merge_config(struct branch *branch);\n-- \n2.25.1.947.ga5bc3d07fe\n\n"},{"id":"392741","messageId":"20200302134842.GB1176622@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200301220531.iuokzzdb5gruslrn@doriath","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-02T13:48:42Z","receivedAt":"2020-03-02T13:48:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 01, 2020 at 11:05:31PM +0100, Damien Robert wrote:\n\n> So it remains the problem of handling the 'upstream' case.\n> The ideal solution would be to not duplicate branch_get_push_1.\n\nYeah, that would be nice (though at least if it's all contained in\nremote.c, we can live with some duplication). There's already some\nduplication in the way remote_ref_for_branch() applies remote refspecs.\n\nAnd I think all of this may be duplicated with git-push itself (which\nwould also be nice to get rid of, but last time I looked into it was\nhard to refactor it to do so).\n\n> In most of the case, this function finds `dst` which is exactly the\n> push:remoteref we are looking for.\n> \n> Then branch_get_push_1 uses\n> \t\tret = tracking_for_push_dest(remote, dst, err);\n> which simply calls\n> \tret = apply_refspecs(&remote->fetch, dst);\n\nRight, there we already have the remote name, and are applying the fetch\nrefspecs to know what our tracking branch would be. So in\nremote_ref_for_branch(), we'd just not apply those.\n\n> The only different case is\n> \tcase PUSH_DEFAULT_UPSTREAM:\n> \t\treturn branch_get_upstream(branch, err);\n> which returns\n> \tbranch->merge[0]->dst\n\nWe also have PUSH_DEFAULT_NOTHING, for which obviously we'd return\nnothing (NULL or an empty string).\n\nLikewise for SIMPLE, we probably need to check that the upstream has a\nmatching name (and return nothing if not).\n\n> So we could almost write an auxiliary function that returns push:remoteref\n> and use it both in remote_ref_for_branch and branch_get_push_1 (via a\n> further call to tracking_for_push_dest) except for the 'upstream' case\n> which is subtly different.\n\nYes, that makes sense.\n\n> In the 'upstream' case, the auxiliary function would return\n> branch->merge_name[0]. So the question is: can\n> tracking_for_push_dest(branch->merge_name[0]) be different from\n> branch->merge[0]->dst?\n\nThose will both return tracking refs. I think you just want\nmerge[0]->src for the upstream case.\n\nAnd yes, the two can be different. It's the same case as when the\nupstream branch has a different name than the current branch.\n\n> Another solution could be as follow: we already store `push` in\n> `branch->push_tracking_ref`. So the question is: can we always easily convert\n> something like refs/remotes/origin/branch_name to refs/heads/branch_name\n> (ie essentially reverse ètracking_for_push_dest`), or are there corner cases?\n\nThis would basically be reverse-applying the fetch refspec. In theory\nit should be possible, but there are cases where somebody has\noverlapping refspecs. But at any rate, I think it's better to just get\nthe pre-mapped values (i.e., avoid calling tracking_for_push_dest() in\nthe first place).\n\n> Otherwise a simple but not elegant solution would be to copy paste the\n> code of branch_get_push_1 to remote_ref_for_branch, simply removing the\n> calls to `tracking_for_push_dest` and using remote->branch_name[0] rather\n> than remote->branch[0]->dst for the upstream case.\n\nYeah, I think that's going to be the easiest. It would be nice to avoid\nrepeating that switch(), but frankly I think the boilerplate you'll end\nup with trying to handle the two cases may be worse than just repeating\nit. It may be worth adding a comment to each function to mention the\nother, and that any changes need to match.\n\n-Peff\n"},{"id":"392786","messageId":"20200303161223.1870298-1-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":"20200302133217.GA1176622@coredump.intra.peff.net","subject":"[PATCH v2 0/2]","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:12:21Z","receivedAt":"2020-03-03T16:12:58Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"Here is the version 2. I incorporated Jeff's preliminary patch, and handled\nall push.default cases and added tests for them.\n\nDamien Robert (1):\n remote.c: fix handling of %(push:remoteref)\n\nJeff King (1):\n remote: drop \"explicit\" parameter from remote_ref_for_branch()\n\n ref-filter.c | 6 +--\n remote.c | 113 +++++++++++++++++++++++++++++-----------\n remote.h | 3 +-\n t/t6300-for-each-ref.sh | 29 ++++++++++-\n 4 files changed, 115 insertions(+), 36 deletions(-)\n\n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392787","messageId":"20200303161223.1870298-2-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":"20200303161223.1870298-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v2 1/2] remote: drop \"explicit\" parameter from remote_ref_for_branch()","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:12:22Z","receivedAt":"2020-03-03T16:12:58Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From: Jeff King \n\nCommit 9700fae5ee (for-each-ref: let upstream/push report the remote ref\nname, 2017-11-07) added a remote_ref_for_branch() helper, which is\nmodeled after remote_for_branch(). This includes providing an \"explicit\"\nout-parameter that tells the caller whether the remote was configured by\nthe user, or whether we picked a default name like \"origin\".\n\nBut unlike remote names, there's no default case for the remote branch\nname. In any case where we don't set \"explicit\", we'd just an empty\nstring anyway. Let's instead return NULL in this case, letting us\nsimplify the function interface.\n\nSigned-off-by: Jeff King \nSigned-off-by: Damien Robert \n---\n ref-filter.c | 6 ++----\n remote.c | 11 ++---------\n remote.h | 3 +--\n 3 files changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6867e33648..9837700732 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1459,12 +1459,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\tremote_for_branch(branch, &explicit);\n \t\t*s = xstrdup(explicit ? remote : \"\");\n \t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n-\t\tint explicit;\n \t\tconst char *merge;\n \n-\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n-\t\t\t\t\t &explicit);\n-\t\t*s = xstrdup(explicit ? merge : \"\");\n+\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push);\n+\t\t*s = xstrdup(merge ? merge : \"\");\n \t} else\n \t\tBUG(\"unhandled RR_* enum\");\n }\ndiff --git a/remote.c b/remote.c\nindex 593ce297ed..c43196ec06 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -516,14 +516,11 @@ 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-\t\t\t\t int *explicit)\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\tif (explicit)\n-\t\t\t\t\t*explicit = 1;\n \t\t\t\treturn branch->merge_name[0];\n \t\t\t}\n \t\t} else {\n@@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\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\tif (explicit)\n-\t\t\t\t\t*explicit = 1;\n \t\t\t\treturn dst;\n \t\t\t}\n \t\t}\n \t}\n-\tif (explicit)\n-\t\t*explicit = 0;\n-\treturn \"\";\n+\treturn NULL;\n }\n \n static struct remote *remote_get_1(const char *name,\ndiff --git a/remote.h b/remote.h\nindex b134cc21be..11d8719b58 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -261,8 +261,7 @@ struct branch {\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-const char *remote_ref_for_branch(struct branch *branch, int for_push,\n-\t\t\t\t int *explicit);\n+const char *remote_ref_for_branch(struct branch *branch, int for_push);\n \n /* returns true if the given branch has merge configuration given. */\n int branch_has_merge_config(struct branch *branch);\n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392788","messageId":"20200303161223.1870298-3-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":"20200303161223.1870298-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:12:23Z","receivedAt":"2020-03-03T16:13:00Z","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\nSigned-off-by: Damien Robert \n---\n remote.c | 106 +++++++++++++++++++++++++++++++---------\n t/t6300-for-each-ref.sh | 29 ++++++++++-\n 2 files changed, 112 insertions(+), 23 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c43196ec06..b3ce992d01 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@@ -1656,6 +1634,76 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\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\tchar *dst;\n+\n+\t\tdst = apply_refspecs(&remote->push, branch->refname);\n+\t\tif (!dst)\n+\t\t\treturn NULL;\n+\n+\t\treturn dst;\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\t{\n+\t\t\tif (!branch || !branch->merge ||\n+\t\t\t !branch->merge[0] || !branch->merge[0]->dst)\n+\t\t\treturn NULL;\n+\n+\t\t\treturn branch->merge[0]->src;\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\tup = branch_get_upstream(branch, NULL);\n+\t\t\tif (!up)\n+\t\t\t\treturn NULL;\n+\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n+\t\t\tif (!cur)\n+\t\t\t\treturn NULL;\n+\t\t\tif (strcmp(cur, up))\n+\t\t\t\treturn NULL;\n+\n+\t\t\treturn branch->refname;\n+\t\t}\n+\t}\n+\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+ * 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@@ -1735,6 +1783,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 9c910ce746..60e21834fd 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '\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\ttest from, = \"$actual\" &&\n+\t\tgit config branch.push-simple.remote from &&\n+\t\tgit config branch.push-simple.merge refs/heads/master &&\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\tactual=\"$(git -c push.default=upstream for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing 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\tgit config branch.push-simple.merge refs/heads/push-simple &&\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,refs/heads/push-simple = \"$actual\"\n \t)\n '\n \n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392789","messageId":"20200303161606.xe5iof6hz2nubc7t@feanor","threadId":"52910","inReplyTo":"20200302133217.GA1176622@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:16:06Z","receivedAt":"2020-03-03T16:16:13Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Jeff King, Mon 02 Mar 2020 at 08:32:17 (-0500) :\n> > I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the\n> > situation is handled by RR_REMOTE_REF, where explicit is not used at all.\n> > So we could remove it.\n> \n> We do look at it, but it's pointless to do so:\n\nOh sorry, I don't know how I missed this line while I saw it above.\n> \n> $ git grep -hn -C4 remote_ref_for_branch origin:ref-filter.c\n> 1461-\t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n> 1462-\t\tint explicit;\n> 1463-\t\tconst char *merge;\n> 1464-\n> 1465:\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n> 1466-\t\t\t\t\t &explicit);\n> 1467-\t\t*s = xstrdup(explicit ? merge : \"\");\n> 1468-\t} else\n> 1469-\t\tBUG(\"unhandled RR_* enum\");\n> \n> I think we probably ought to do this as a preparatory patch in your\n> series.\n\nI wonder about the case of RR_REMOTE_NAME to.\nWe always have explicit=1, except if we fallback all the way to 'origin',\nvia pushremote_for_branch and then remote_for_branch. But 'origin' even\nthrough it is implicit, is still the name of the remote we fetch/push to by\ndefault. So should not %(push), %(upstream) still show origin in this case?\n"},{"id":"392790","messageId":"20200303162514.dkmulpq5fw3t6hpt@feanor","threadId":"52910","inReplyTo":"20200302134842.GB1176622@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:25:14Z","receivedAt":"2020-03-03T16:25:19Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Jeff King, Mon 02 Mar 2020 at 08:48:42 (-0500) :\n> And I think all of this may be duplicated with git-push itself (which\n> would also be nice to get rid of, but last time I looked into it was\n> hard to refactor it to do so).\n\nI had a quick look at git-push but the duplication does not seems too bad.\n\n> > In the 'upstream' case, the auxiliary function would return\n> > branch->merge_name[0]. So the question is: can\n> > tracking_for_push_dest(branch->merge_name[0]) be different from\n> > branch->merge[0]->dst?\n\n> Those will both return tracking refs. I think you just want\n> merge[0]->src for the upstream case.\n> And yes, the two can be different. It's the same case as when the\n> upstream branch has a different name than the current branch.\n\nI meant, now that we have branch_get_push_remoteref, can we replace\nthe body of branch_get_push_1 by\n\tremote = remote_get(pushremote_for_branch(branch, NULL));\n\tret = tracking_for_push_dest(remote, branch_get_push_remoteref(branch), err);\n(we would need to add error handling in branch_get_push_remoteref but that\nis easy)\n\nCurrently that is exactly what branch_get_push_1 does, except in the\nPUSH_DEFAULT_UPSTREAM where it returns branch->merge[0]->dst.\nBut branch->merge is set up in `set_merge`, where we have:\n\t\tret->merge[i]->src = xstrdup(ret->merge_name[i]);\n\t\t...\n\t\tif (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),\n\t\t\t &oid, &ref) == 1)\n\t\t\tret->merge[i]->dst = ref;\nSo my question was: can dwim_ref(branch->merge[0]->src) be different from\ntracking_for_push_dest(branch->merge[0]->src)?\n\n> Yeah, I think that's going to be the easiest. It would be nice to avoid\n> repeating that switch(), but frankly I think the boilerplate you'll end\n> up with trying to handle the two cases may be worse than just repeating\n> it.\n\nThat's what I went with. We can always refactorise branch_get_push_1 to use\nbranch_get_push_remoteref afterwards.\n\n> It may be worth adding a comment to each function to mention the\n> other, and that any changes need to match.\n\nI tried to add a comment, but I don't know if it is helpful enough.\n"},{"id":"392791","messageId":"20200303162906.xadbaeaq4nurqsem@feanor","threadId":"52910","inReplyTo":"20200303161223.1870298-3-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T16:29:06Z","receivedAt":"2020-03-03T16:29:11Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"I have some remarks/questions:\n\nFrom Damien Robert, Tue 03 Mar 2020 at 17:12:23 (+0100) :\n> +\tif (remote->push.nr) {\n> +\t\tchar *dst;\n> +\t\tdst = apply_refspecs(&remote->push, branch->refname);\n> +\t\tif (!dst)\n> +\t\t\treturn NULL;\n> +\t\treturn dst;\n> +\t}\n\nShould I simply `return apply_refspecs(&remote->push, branch->refname);`\nhere, or is it a good form to always check for a NULL return value even if\nwe do nothing with it?\n\n> +\tcase PUSH_DEFAULT_MATCHING:\n> +\tcase PUSH_DEFAULT_CURRENT:\n> +\t\treturn branch->refname;\n\nHere I follow the logic of branch_get_push1, but the case of\npush.default=matching is not quite correct, because we never check\nthat we have a matching remote branch. On the other hand we cannot check\nthis until we contact the remote, so I don't know how we could get around\nthat.\n\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"392805","messageId":"xmqqzhcx8gz8.fsf@gitster-ct.c.googlers.com","threadId":"52910","inReplyTo":"20200303161223.1870298-2-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v2 1/2] remote: drop \"explicit\" parameter from remote_ref_for_branch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T17:51:07Z","receivedAt":"2020-03-03T18:10:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> From: Jeff King \n>\n> Commit 9700fae5ee (for-each-ref: let upstream/push report the remote ref\n> name, 2017-11-07) added a remote_ref_for_branch() helper, which is\n> modeled after remote_for_branch(). This includes providing an \"explicit\"\n> out-parameter that tells the caller whether the remote was configured by\n> the user, or whether we picked a default name like \"origin\".\n>\n> But unlike remote names, there's no default case for the remote branch\n> name.\n\nUp to this point, it is well written and easy to read. I think\n\"there is no case where a default name for the remoate branch is\nused\" would be even more easy to read.\n\nIn any case, if there is no case that default name, I understand\nthat explicit is always set to 1?\n\n> In any case where we don't set \"explicit\", we'd just an empty\n> string anyway.\n\nSorry, but I cannot parse this. But earlier, you established that\nthere is no case that a default is used, so is there any case where\nwe don't set \"explicit\"? I don't get it.\n\n> Let's instead return NULL in this case, letting us\n> simplify the function interface.\n\n> \t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n> -\t\tint explicit;\n> \t\tconst char *merge;\n> \n> -\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n> -\t\t\t\t\t &explicit);\n> -\t\t*s = xstrdup(explicit ? merge : \"\");\n> +\t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push);\n> +\t\t*s = xstrdup(merge ? merge : \"\");\n\nMental note: as long as explicit is set to true when the function\nreturns a non-null value, this change will be a no-op as far as the\nresult is concerned, which is a good thing.\n\n> diff --git a/remote.c b/remote.c\n> index 593ce297ed..c43196ec06 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -516,14 +516,11 @@ 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> -\t\t\t\t int *explicit)\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\tif (explicit)\n> -\t\t\t\t\t*explicit = 1;\n> \t\t\t\treturn branch->merge_name[0];\n\nThis case returns a non-NULL pointer, and used to set explicit to\ntrue.\n\n> \t\t\t}\n> \t\t} else {\n> @@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,\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\tif (explicit)\n> -\t\t\t\t\t*explicit = 1;\n> \t\t\t\treturn dst;\n\nSo did this.\n\n> \t\t\t}\n> \t\t}\n> \t}\n> -\tif (explicit)\n> -\t\t*explicit = 0;\n> -\treturn \"\";\n> +\treturn NULL;\n\nAnd this one used to return 0 in explicit and returned an empty\nstring. The only caller ignored that empty string so returning NULL\nin the new code does not make a difference, which also is a good\nthing.\n\nSo the code looks correctly done. I just don't get the feeling that\nthe proposed log message explains the change clearly to readers.\n\nAfter reading the code through, I think \"there's no default case\"\nwas what caused my confusion.\n\n But unlike remote names, there is no default name when the user\n didn't configure one. The only way the \"explicit\" parameter is\n used by the caller is to use the value returned from the helper\n when it is set, and use an empty string otherwise, ignoring the\n returned value from the helper.\n\n Let's drop the \"explicit\" out-parameter, and return NULL when\n the returned value from the helper should be ignored, to\n simplify the function interface.\n\nperhaps?\n"},{"id":"392807","messageId":"xmqqtv358fkk.fsf@gitster-ct.c.googlers.com","threadId":"52910","inReplyTo":"20200303161223.1870298-3-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T18:21:31Z","receivedAt":"2020-03-03T18:21:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> Looking at the value of %(push:remoteref) only handles the case when an\n> explicit push refspec is passed. But it does not handle the fallback\n> cases of looking at the configuration value of `push.default`.\n>\n> In 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>\n> prints a useful tracking ref for the first for-each-ref, but an empty\n> string for the second.\n>\n> Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:\n> let upstream/push report the remote ref name) is to get exactly which\n> branch `git push` will push to, even in the fallback cases, fix this.\n>\n> To 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>\n> We also update t/6300-for-each-ref.sh to handle all `push.default`\n> strategies. This involves testing `push.default=simple` twice, once\n> where there is a matching upstream branch and once when there is none.\n>\n> Signed-off-by: Damien Robert \n> ---\n> remote.c | 106 +++++++++++++++++++++++++++++++---------\n> t/t6300-for-each-ref.sh | 29 ++++++++++-\n> 2 files changed, 112 insertions(+), 23 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index c43196ec06..b3ce992d01 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\nMental note: this function was moved down, the main part of the\nlogic extracted to a new branch_get_push_remoteref() helper, which\nin addition got extended.\n\n> @@ -1656,6 +1634,76 @@ static const char *tracking_for_push_dest(struct remote *remote,\n> \treturn ret;\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\tchar *dst;\n> +\n> +\t\tdst = apply_refspecs(&remote->push, branch->refname);\n> +\t\tif (!dst)\n> +\t\t\treturn NULL;\n> +\n> +\t\treturn dst;\n> +\t}\n\nThat's a fairly expensive way to write\n\n\tif (remote->push.nr)\n\t\treturn apply_refspecs(&remote->push, branch->refname);\n\none-liner.\n\nIn any case, up to this point, the code does exactly the same thing\nas the original (i.e. when remote..push is set and\ncovers the current branch, use that to figure out which way we are\npushing).\n\n> +\tif (remote->mirror)\n> +\t\treturn branch->refname;\n\nIf mirroring, we push to the same name, OK.\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\nThese three cases are straight-forward, I think.\n\n> +\tcase PUSH_DEFAULT_UPSTREAM:\n> +\t\t{\n> +\t\t\tif (!branch || !branch->merge ||\n> +\t\t\t !branch->merge[0] || !branch->merge[0]->dst)\n> +\t\t\treturn NULL;\n> +\n> +\t\t\treturn branch->merge[0]->src;\n> +\t\t}\n\nThis is strangely indented and somewhat unreadable. Why isn't this\nmore like:\n\n\tcase PUSH_DEFAULT_UPSTREAM:\n\t\tif (branch && branch->merge && branch->merge[0] &&\n\t\t branch->merge[0]->dst)\n\t\t\treturn branch->merge[0]->src;\n\t\tbreak;\n\nand have \"return NULL\" after the switch() statement before we leave\nthe function?\n\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, NULL);\n> +\t\t\tif (!up)\n> +\t\t\t\treturn NULL;\n> +\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n> +\t\t\tif (!cur)\n> +\t\t\t\treturn NULL;\n> +\t\t\tif (strcmp(cur, up))\n> +\t\t\t\treturn NULL;\n\nThis is probably not all that performance critical, so\n\n\t\t\tup = branch_get_upstream(branch, NULL);\n\t\t\tcurrent = tracking_for_push_dest(remote, branch->refname, NULL);\n\t\t\tif (!up || !current || strcmp(current, up))\n\t\t\t\treturn NULL;\n\nmight be easier to follow.\n\n> +\t\t\treturn branch->refname;\n\n> +\t\t}\n> +\t}\n> +\n> +\tBUG(\"unhandled push situation\");\n\nThis is better done / easier to read inside switch() as default:\nclause.\n\nBy the way, I have a bit higher-level question. \n\nAll of the above logic that decides what should happen in \"git push\"\nMUST have existing code we already use to implement \"git push\", no?\n\nWhy do we need to reinvent it here, instead of reusing the existing\ncode? Is it because the interface into the functions that implement\nthe existing logic is very different from what this function wants?\n\n"},{"id":"392808","messageId":"xmqqpndt8f7p.fsf@gitster-ct.c.googlers.com","threadId":"52910","inReplyTo":"20200303162906.xadbaeaq4nurqsem@feanor","subject":"Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T18:29:14Z","receivedAt":"2020-03-03T18:29:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n>> +\tcase PUSH_DEFAULT_MATCHING:\n>> +\tcase PUSH_DEFAULT_CURRENT:\n>> +\t\treturn branch->refname;\n>\n> Here I follow the logic of branch_get_push1, but the case of\n> push.default=matching is not quite correct, because we never check\n> that we have a matching remote branch. On the other hand we cannot check\n> this until we contact the remote, so I don't know how we could get around\n> that.\n\nQuite honestly, I do not think that is a problem that needs to be\nsolved; there is no workable definition.\n\n"},{"id":"392814","messageId":"20200303211142.GA36275@coredump.intra.peff.net","threadId":"52910","inReplyTo":"xmqqzhcx8gz8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] remote: drop \"explicit\" parameter from remote_ref_for_branch()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-03T21:11:42Z","receivedAt":"2020-03-03T21:11:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:\n\n> > But unlike remote names, there's no default case for the remote branch\n> > name.\n> \n> Up to this point, it is well written and easy to read. I think\n> \"there is no case where a default name for the remoate branch is\n> used\" would be even more easy to read.\n> \n> In any case, if there is no case that default name, I understand\n> that explicit is always set to 1?\n> \n> > In any case where we don't set \"explicit\", we'd just an empty\n> > string anyway.\n> \n> Sorry, but I cannot parse this. But earlier, you established that\n> there is no case that a default is used, so is there any case where\n> we don't set \"explicit\"? I don't get it.\n\nMaybe more clear:\n\nFor remote names, we will always return one of two things:\n\n - a remote name based on user config, in which case explicit=1\n\n - the default string \"origin\", in which case explicit=0\n\nFor remote branches, we will return either:\n\n - the remote branch name from config, in which case explicit=1\n\n - the empty string, in which case explicit=0\n\nBut nobody ever looks at that empty string. For the second case, we\ncould just as well return NULL. At which point we don't need an explicit\nflag at all, as the caller can just check for NULL.\n\n(written before reading what you wrote below)\n\n> After reading the code through, I think \"there's no default case\"\n> was what caused my confusion.\n> \n> But unlike remote names, there is no default name when the user\n> didn't configure one. The only way the \"explicit\" parameter is\n> used by the caller is to use the value returned from the helper\n> when it is set, and use an empty string otherwise, ignoring the\n> returned value from the helper.\n> \n> Let's drop the \"explicit\" out-parameter, and return NULL when\n> the returned value from the helper should be ignored, to\n> simplify the function interface.\n\nYes, that looks fine to me.\n\n-Peff\n"},{"id":"392824","messageId":"xmqqeeu96pul.fsf@gitster-ct.c.googlers.com","threadId":"52910","inReplyTo":"20200303211142.GA36275@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] remote: drop \"explicit\" parameter from remote_ref_for_branch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T22:22:26Z","receivedAt":"2020-03-03T22:22:35Z","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 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:\n>\n>> > But unlike remote names, there's no default case for the remote branch\n>> > name.\n>> \n>> Up to this point, it is well written and easy to read. I think\n>> \"there is no case where a default name for the remoate branch is\n>> used\" would be even more easy to read.\n>> \n>> In any case, if there is no case that default name, I understand\n>> that explicit is always set to 1?\n>> \n>> > In any case where we don't set \"explicit\", we'd just an empty\n>> > string anyway.\n>> \n>> Sorry, but I cannot parse this. But earlier, you established that\n>> there is no case that a default is used, so is there any case where\n>> we don't set \"explicit\"? I don't get it.\n>\n> Maybe more clear:\n> ...\n> Yes, that looks fine to me.\n\nThanks for a clarification.\n"},{"id":"392825","messageId":"20200303222423.wfbjuuwp3263qesv@doriath","threadId":"52910","inReplyTo":"xmqqtv358fkk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-03T22:24:23Z","receivedAt":"2020-03-03T22:24:29Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Junio C Hamano, Tue 03 Mar 2020 at 10:21:31 (-0800) :\n> Mental note: this function was moved down, the main part of the\n> logic extracted to a new branch_get_push_remoteref() helper, which\n> in addition got extended.\n\nExactly.\n\n> That's a fairly expensive way to write\n> \tif (remote->push.nr)\n> \t\treturn apply_refspecs(&remote->push, branch->refname);\n> one-liner.\n\nYou anticipated the question I asked afterwards :)\n\n> > +\tcase PUSH_DEFAULT_UPSTREAM:\n> > +\t\t{\n> > +\t\t\tif (!branch || !branch->merge ||\n> > +\t\t\t !branch->merge[0] || !branch->merge[0]->dst)\n> > +\t\t\treturn NULL;\n> > +\n> > +\t\t\treturn branch->merge[0]->src;\n> > +\t\t}\n> \n> This is strangely indented and somewhat unreadable. Why isn't this\n> more like:\n\nSorry I missed the indentation for the return NULL.\n \n> \tcase PUSH_DEFAULT_UPSTREAM:\n> \t\tif (branch && branch->merge && branch->merge[0] &&\n> \t\t branch->merge[0]->dst)\n> \t\t\treturn branch->merge[0]->src;\n> \t\tbreak;\n> \n> and have \"return NULL\" after the switch() statement before we leave\n> the function?\n\nWe could, I agree this is more readable. If we return NULL after the\nswitch, we need to put the\n> > +\tBUG(\"unhandled push situation\")\nin a default clause a you say.\n\nThe reason I wrote the code as above is to be as close as possible to\n`branch_get_push_1`. If we make the changes you suggest, I'll probably need\na preliminary patch to change `branch_get_push_1` accordingly.\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, NULL);\n> > +\t\t\tif (!up)\n> > +\t\t\t\treturn NULL;\n> > +\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n> > +\t\t\tif (!cur)\n> > +\t\t\t\treturn NULL;\n> > +\t\t\tif (strcmp(cur, up))\n> > +\t\t\t\treturn NULL;\n> \n> This is probably not all that performance critical, so\n> \t\t\tup = branch_get_upstream(branch, NULL);\n> \t\t\tcurrent = tracking_for_push_dest(remote, branch->refname, NULL);\n> \t\t\tif (!up || !current || strcmp(current, up))\n> \t\t\t\treturn NULL;\n> might be easier to follow.\n\nI don't mind but likewise in this case we should probably change\nbranch_get_push_1 too.\n\n> By the way, I have a bit higher-level question. \n> \n> All of the above logic that decides what should happen in \"git push\"\n> MUST have existing code we already use to implement \"git push\", no?\n\nYes.\n\n> Why do we need to reinvent it here, instead of reusing the existing\n> code? Is it because the interface into the functions that implement\n> the existing logic is very different from what this function wants?\n\nMostly yes. The logic of git push is to massage the refspecs directly, for\ninstance:\n\tcase PUSH_DEFAULT_MATCHING:\n\t\trefspec_append(&rs, \":\");\n\tcase PUSH_DEFAULT_CURRENT:\n\t\t...\n\t\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->refname);\n\tcase PUSH_DEFAULT_UPSTREAM:\n\t\t...\n\t\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->merge[0]->src);\n\nAnd the error messages are also not the same, and to give a good error\nmessage we need to parse the different cases.\n\nIt may be possible to refactorize all this, but not in an obvious way and\nit would be a lot more work than this patch series.\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"392829","messageId":"xmqq5zfl6omm.fsf@gitster-ct.c.googlers.com","threadId":"52910","inReplyTo":"20200303222423.wfbjuuwp3263qesv@doriath","subject":"Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T22:48:49Z","receivedAt":"2020-03-03T22:48:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n>> By the way, I have a bit higher-level question. \n>> \n>> All of the above logic that decides what should happen in \"git push\"\n>> MUST have existing code we already use to implement \"git push\", no?\n>\n> Yes.\n>\n>> Why do we need to reinvent it here, instead of reusing the existing\n>> code? Is it because the interface into the functions that implement\n>> the existing logic is very different from what this function wants?\n>\n> Mostly yes. The logic of git push is to massage the refspecs directly, for\n> instance:\n> \tcase PUSH_DEFAULT_MATCHING:\n> \t\trefspec_append(&rs, \":\");\n> \tcase PUSH_DEFAULT_CURRENT:\n> \t\t...\n> \t\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->refname);\n> \tcase PUSH_DEFAULT_UPSTREAM:\n> \t\t...\n> \t\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->merge[0]->src);\n>\n> And the error messages are also not the same, and to give a good error\n> message we need to parse the different cases.\n>\n> It may be possible to refactorize all this, but not in an obvious way and\n> it would be a lot more work than this patch series.\n\nYeah, in light of the analysis I agree it makes sense to take the\napproach of these two patches, at least for now.\n\nThanks.\n"},{"id":"393135","messageId":"20200312164558.2388589-1-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":"20200303161223.1870298-3-damien.olivier.robert+git@gmail.com","subject":"[PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-12T16:45:58Z","receivedAt":"2020-03-12T16:46:35Z","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\nSigned-off-by: Damien Robert \n---\n\nI ended up following most of Junio's suggestion, except having a\n default: BUG(...)\nand returning NULL at the end of the case.\n\nI prefer to return explicitly in each case statement rather than use break\nto fallback at the end of the case.\n\nI said I would also update branch_get_push1 to be as similar as possible to\nbranch_get_push_remoteref, but because of the error handling of the latter,\nit would makes the syntax a bit weird, so I did not touch it.\n\nI am still a bit annoyed that I cannot call branch_get_push_remoteref from\nbranch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can\nwait and we will need to work with the code duplication meanwhile.\n\n remote.c | 94 +++++++++++++++++++++++++++++++----------\n t/t6300-for-each-ref.sh | 29 ++++++++++++-\n 2 files changed, 100 insertions(+), 23 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c43196ec06..352ea240cd 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@@ -1656,6 +1634,64 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\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 (branch && branch->merge && branch->merge[0] &&\n+\t\t branch->merge[0]->dst)\n+\t\t\treturn branch->merge[0]->src;\n+\t\telse\n+\t\t\treturn NULL;\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, NULL);\n+\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n+\t\t\tif (up && cur && !strcmp(cur, up))\n+\t\t\t\treturn branch->refname;\n+\t\t\telse\n+\t\t\t\treturn NULL;\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+ * 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@@ -1735,6 +1771,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 9c910ce746..60e21834fd 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '\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\ttest from, = \"$actual\" &&\n+\t\tgit config branch.push-simple.remote from &&\n+\t\tgit config branch.push-simple.merge refs/heads/master &&\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\tactual=\"$(git -c push.default=upstream for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing 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\tgit config branch.push-simple.merge refs/heads/push-simple &&\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,refs/heads/push-simple = \"$actual\"\n \t)\n '\n \n-- \nPatched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)\n\n"},{"id":"394028","messageId":"20200325221614.ekn56wamfgs4bwmq@doriath","threadId":"52910","inReplyTo":"20200312164558.2388589-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-25T22:16:14Z","receivedAt":"2020-03-25T22:16:20Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"Hi Junio,\n\nIMHO this patch should be good to cook.\n\nThanks!\n"},{"id":"394208","messageId":"xmqqblohe9ip.fsf@gitster.c.googlers.com","threadId":"52910","inReplyTo":"20200325221614.ekn56wamfgs4bwmq@doriath","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T22:08:14Z","receivedAt":"2020-03-27T22:08:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> IMHO this patch should be good to cook.\n\nWould love to queue it but I haven't had a time to look at it.\n\nThanks.\n\n\n"},{"id":"394224","messageId":"20200328131553.GA643242@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200312164558.2388589-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T13:15:53Z","receivedAt":"2020-03-28T13:15:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2020 at 05:45:58PM +0100, Damien Robert wrote:\n\n> Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:\n> let upstream/push report the remote ref name) is to get exactly which\n> branch `git push` will push to, even in the fallback cases, fix this.\n> \n> To 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\nI looked at this again with fresh eyes, and I think it's a pretty\npractical fix all around. I noticed one memory leak, but it's actually\nthere already. :-/\n\n> I ended up following most of Junio's suggestion, except having a\n> default: BUG(...)\n> and returning NULL at the end of the case.\n>\n> I prefer to return explicitly in each case statement rather than use break\n> to fallback at the end of the case.\n\nWhat you have looks reasonable to me (and would hopefully get us a\ncompiler warning if new push modes are added).\n\n> I said I would also update branch_get_push1 to be as similar as possible to\n> branch_get_push_remoteref, but because of the error handling of the latter,\n> it would makes the syntax a bit weird, so I did not touch it.\n>\n> I am still a bit annoyed that I cannot call branch_get_push_remoteref from\n> branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can\n> wait and we will need to work with the code duplication meanwhile.\n\nI looked into this, too, and have a working patch. It does get a little\nawkward, though, and I'm happy to just take your patch for now as the\npractical thing.\n\n> -const char *remote_ref_for_branch(struct branch *branch, int for_push)\n> [...]\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\nThis is the leak in the old code. apply_refspecs() returns a newly\nallocated buffer, but our caller would never know to free it because we\nreturn a const pointer.\n\nAnd we have the same problem in the new code:\n\n> +static const char *branch_get_push_remoteref(struct branch *branch)\n> [...]\n> +\tif (remote->push.nr) {\n> +\t\treturn apply_refspecs(&remote->push, branch->refname);\n> +\t}\n\nBut we can't return a \"char *\", because all of the other return values\npoint to long-lived strings that the caller won't own. One way to solve\nit would be to xstrdup() all of those. I'm not thrilled about that,\nthough; for-each-ref already does way more allocations-per-ref than I'd\nlike.\n\nA better solution would be for this function to write the result into a\nstrbuf. For one-off calls that's no worse than allocating a string to\nreturn, and for repeated calls like for-each-ref, it could reuse the\nsame allocated strbuf over and over.\n\nSince this leak existed before your patch, I'm inclined to treat it as a\nseparate topic and not have it hold up this fix.\n\n> +static const char *branch_get_push_remoteref(struct branch *branch)\n> [...]\n\nAll the logic here makes sense to me.\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n\nThe tests makes sense to me, though I found a few nits to pick:\n\n> index 9c910ce746..60e21834fd 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '\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\ttest from, = \"$actual\" &&\n\nThe existing tests just assume taht push.default=simple. Since we're now\ntesting everything, should this be \"git -c push.default=simple\" to be\nmore explicit?\n\nLikewise, is it worth labeling all of the simple cases with a comment\n(or possibly putting them in separate tests, though I guess some of the\nsetup is shared)? This one expects blank because there's no configured\nupstream.\n\n> +\t\tgit config branch.push-simple.remote from &&\n> +\t\tgit config branch.push-simple.merge refs/heads/master &&\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\nThis one expects blank because the upstream and local names don't match.\n\n> +\t\tactual=\"$(git -c push.default=upstream for-each-ref \\\n> +\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n> +\t\t\trefs/heads/push-simple)\" &&\n> +\t\ttest from,refs/heads/master = \"$actual\" &&\n\nThis one has a real configured upstream (and relies on the\nbranch.*.merge config set above). OK.\n\nIt's a little funny that the branch is still called push-simple. :)\n\n> +\t\tactual=\"$(git -c push.default=current for-each-ref \\\n> +\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n> +\t\t\trefs/heads/push-simple)\" &&\n> +\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n\nSame name on the other side. OK.\n\n> +\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n> +\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n> +\t\t\trefs/heads/push-simple)\" &&\n> +\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n\nDitto for matching, which I think is the only sensible output.\n\n> +\t\tactual=\"$(git -c push.default=nothing 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\nNothing for nothing. Makes sense.\n\n> +\t\tgit config branch.push-simple.merge refs/heads/push-simple &&\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,refs/heads/push-simple = \"$actual\"\n\nAnd this is a real simple that actually shows something. It would make\nmore sense to me with the other \"simple\" tests, but I guess _not_ having\nthe upstream set to the same name is important for the quality of the\n\"current\" and \"upstream\" tests.\n\nMaybe we could do this test first, before setting branch.*.merge to\nrefs/heads/master?\n\n-Peff\n"},{"id":"394226","messageId":"20200328133134.GA1196665@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200328131553.GA643242@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T13:31:34Z","receivedAt":"2020-03-28T13:31:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 09:15:53AM -0400, Jeff King wrote:\n\n> > I said I would also update branch_get_push1 to be as similar as possible to\n> > branch_get_push_remoteref, but because of the error handling of the latter,\n> > it would makes the syntax a bit weird, so I did not touch it.\n> >\n> > I am still a bit annoyed that I cannot call branch_get_push_remoteref from\n> > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can\n> > wait and we will need to work with the code duplication meanwhile.\n> \n> I looked into this, too, and have a working patch. It does get a little\n> awkward, though, and I'm happy to just take your patch for now as the\n> practical thing.\n\nHere's what I came up with (against master, but I stole a few bits from\nyour patch to connect it to remote_ref_for_branch and test it). I'll\nquote bits of it to comment inline, and you can find the complete patch\nat the bottom.\n\n> @@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch,\n> }\n> \n> __attribute__((format (printf,2,3)))\n> -static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n> +static void *error_buf(struct strbuf *err, const char *fmt, ...)\n> {\n> \tif (err) {\n> \t\tva_list ap;\n\nThis loosens up error_buf() to make it usable for functions that aren't\nreturning a string. Which we use for...\n\n> @@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n> \treturn NULL;\n> }\n> \n> -const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n> +struct refspec_item *branch_get_upstream_refspec(struct branch *branch,\n> +\t\t\t\t\t\t struct strbuf *err)\n> {\n> \tif (!branch)\n> \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> @@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *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> \n> -\treturn branch->merge[0]->dst;\n> +\treturn branch->merge[0];\n> +}\n> +\n> +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n> +{\n> +\tstruct refspec_item *ret = branch_get_upstream_refspec(branch, err);\n> +\tif (ret)\n> +\t\treturn ret->dst;\n> +\treturn NULL;\n> }\n> \n\nWe can't use branch_get_upstream() to get the remote side, since it\nreturns branch->merge[0]->dst, and we'd want branch->merge[0]->src. So\nthis factors out a helper that returns both sides, and\nbranch_get_upstream() can pick out \"dst\".\n\n> @@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote,\n> \treturn ret;\n> }\n> \n> -static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n> +static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err)\n> {\n> \tstruct remote *remote;\n> \n\nHere I was able to just convert push_1 into push_remoteref, since it has\nall of the error-handling we want (and the error_buf() helper makes it\nsafe to pass a NULL and just ignore the errors if a caller wants to).\n\nAnd then push_1 essentially becomes:\n\n const char *remoteref = branch_get_push_remoteref(branch, err);\n return tracking_for_push_dest(remote, remoteref, err);\n\n> @@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n> \t\t\t\t branch->name);\n> \n> \tif (remote->push.nr) {\n> -\t\tchar *dst;\n> -\t\tconst char *ret;\n> -\n> -\t\tdst = apply_refspecs(&remote->push, branch->refname);\n> +\t\tchar *dst = apply_refspecs(&remote->push, 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\treturn dst;\n> \t}\n\nWe're really just dropping the tracking_for_push_dest() here, since we\nwant the remote side. This matches what your patch did, except that we\nhave the error handling.\n\nBy the way, this is how I noticed the memory leak. And this patch does\nmake it worse because we used to get the cleanup of \"dst\" right, but now\nit will be split across two functions and we won't free it.\n\nFor that matter, tracking_for_push_dest() also returns an allocated\nstring as a \"const char *\", so it's a leak, too. Maybe the strbuf plan\nis worth pursuing. :)\n\n> \tif (remote->mirror)\n> -\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n> +\t\treturn branch->refname;\n\nAgain, just dropping the tracking conversion, and it matches your patch.\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\nWe don't need to touch anything here, because both the remote and local\nsides are NULL. :)\n\n> \n> \tcase PUSH_DEFAULT_MATCHING:\n> \tcase PUSH_DEFAULT_CURRENT:\n> -\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n> +\t\treturn branch->refname;\n\nAgain, just dropping tracking.\n\n> \tcase PUSH_DEFAULT_UPSTREAM:\n> -\t\treturn branch_get_upstream(branch, err);\n> +\t\t{\n> +\t\t\tstruct refspec_item *ret =\n> +\t\t\t\tbranch_get_upstream_refspec(branch, err);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret->src;\n> +\t\t\treturn NULL;\n> +\t\t}\n\nHere we have to use the new helper to pull out the \"src\" side. This\nunfortunately means that to get the tracking ref, we'll re-apply\ntracking_for_push_dest(), even though we could have just gotten the\nactual value we wanted from ret->dst (without a memory leak!).\n\n> @@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\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\treturn branch->refname;\n> \t\t}\n\nAnd here again we're less efficient. We already checked the tracking\nhere to make sure we have the same name on both sides. But when we\nreturn, we'll still apply tracking_for_push_dest(), which will just give\nus the same name again (but the caller doesn't know that).\n\n> @@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)\n> \tif (!branch)\n> \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> \n> -\tif (!branch->push_tracking_ref)\n> -\t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n> +\tif (!branch->push_tracking_ref) {\n> +\t\tconst char *remoteref = branch_get_push_remoteref(branch, err);\n> +\t\tif (remoteref) {\n> +\t\t\t/*\n> +\t\t\t * ugh, we have to find remote again; should there be a\n> +\t\t\t * master function which returns both remote and remoteref?\n> +\t\t\t */\n> +\t\t\tstruct remote *remote =\n> +\t\t\t\tremote_get(pushremote_for_branch(branch, NULL));\n> +\t\t\tbranch->push_tracking_ref =\n> +\t\t\t\ttracking_for_push_dest(remote, remoteref, err);\n> +\t\t}\n> +\t}\n> \treturn branch->push_tracking_ref;\n\nAnd here I just dropped push_1 entirely and did it inline. The extra\nremote is ugly though. I guess we could return it as an out-parameter.\n\nSo it does work, but it's kind of awkward. And even if we solved the\nleaking problem by using a strbuf, that would make it doubly awkward,\nbecause the code above would need an _extra_ strbuf to store the\nbranch_get_push_remoteref() result, and only to then convert it via\ntracking_for_push_dest().\n\nFull patch is below if you want to try it out or hack on it further.\n\n---\ndiff --git a/remote.c b/remote.c\nindex c43196ec06..22144c96b5 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@@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch,\n }\n \n __attribute__((format (printf,2,3)))\n-static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n+static void *error_buf(struct strbuf *err, const char *fmt, ...)\n {\n \tif (err) {\n \t\tva_list ap;\n@@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n \treturn NULL;\n }\n \n-const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n+struct refspec_item *branch_get_upstream_refspec(struct branch *branch,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tif (!branch)\n \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n@@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *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 \n-\treturn branch->merge[0]->dst;\n+\treturn branch->merge[0];\n+}\n+\n+const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n+{\n+\tstruct refspec_item *ret = branch_get_upstream_refspec(branch, err);\n+\tif (ret)\n+\t\treturn ret->dst;\n+\treturn NULL;\n }\n \n static const char *tracking_for_push_dest(struct remote *remote,\n@@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n-static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n+static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err)\n {\n \tstruct remote *remote;\n \n@@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n \t\t\t\t branch->name);\n \n \tif (remote->push.nr) {\n-\t\tchar *dst;\n-\t\tconst char *ret;\n-\n-\t\tdst = apply_refspecs(&remote->push, branch->refname);\n+\t\tchar *dst = apply_refspecs(&remote->push, 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\treturn dst;\n \t}\n \n \tif (remote->mirror)\n-\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\t\treturn branch->refname;\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+\t\treturn branch->refname;\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n-\t\treturn branch_get_upstream(branch, err);\n+\t\t{\n+\t\t\tstruct refspec_item *ret =\n+\t\t\t\tbranch_get_upstream_refspec(branch, err);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret->src;\n+\t\t\treturn NULL;\n+\t\t}\n \n \tcase PUSH_DEFAULT_UNSPECIFIED:\n \tcase PUSH_DEFAULT_SIMPLE:\n@@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\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\treturn branch->refname;\n \t\t}\n \t}\n \n@@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)\n \tif (!branch)\n \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n \n-\tif (!branch->push_tracking_ref)\n-\t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n+\tif (!branch->push_tracking_ref) {\n+\t\tconst char *remoteref = branch_get_push_remoteref(branch, err);\n+\t\tif (remoteref) {\n+\t\t\t/*\n+\t\t\t * ugh, we have to find remote again; should there be a\n+\t\t\t * master function which returns both remote and remoteref?\n+\t\t\t */\n+\t\t\tstruct remote *remote =\n+\t\t\t\tremote_get(pushremote_for_branch(branch, NULL));\n+\t\t\tbranch->push_tracking_ref =\n+\t\t\t\ttracking_for_push_dest(remote, remoteref, err);\n+\t\t}\n+\t}\n \treturn branch->push_tracking_ref;\n }\n \n@@ -1735,6 +1734,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, NULL);\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 9c910ce746..60e21834fd 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '\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\ttest from, = \"$actual\" &&\n+\t\tgit config branch.push-simple.remote from &&\n+\t\tgit config branch.push-simple.merge refs/heads/master &&\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\tactual=\"$(git -c push.default=upstream for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/push-simple)\" &&\n+\t\ttest from,refs/heads/push-simple = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing 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\tgit config branch.push-simple.merge refs/heads/push-simple &&\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,refs/heads/push-simple = \"$actual\"\n \t)\n '\n \n"},{"id":"394254","messageId":"20200328222546.gvrtzkcazf3bhjno@doriath","threadId":"52910","inReplyTo":"xmqqblohe9ip.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-28T22:25:46Z","receivedAt":"2020-03-28T22:25:52Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Junio C Hamano, Fri 27 Mar 2020 at 15:08:14 (-0700) :\n> Damien Robert writes:\n> > IMHO this patch should be good to cook.\n> Would love to queue it but I haven't had a time to look at it.\n\nSure, no worries, I was just wondering if this was on your todo list or if\nthe last patch version fell through the cracks (I have no idea how you\nmanage to keep up with the traffic of this ml).\n\nYour message managed to invoke Jeff who suggests some improvements to the\ntest, so I'll reroll anyway :)\n"},{"id":"394848","messageId":"20200406160439.gg5uu6kepnyxpvuc@feanor","threadId":"52910","inReplyTo":"20200328131553.GA643242@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-06T16:04:39Z","receivedAt":"2020-04-06T16:04:48Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Jeff King, Sat 28 Mar 2020 at 09:15:53 (-0400) :\n\n> The tests makes sense to me, though I found a few nits to pick:\n[...]\n\nSo I updated the tests as follow. I put them in another test, as you\nsuggested, this is indeed much clearer.\n\ntest_expect_success ':push:remoteref' '\n\tgit init remote-tests &&\n\t(\n\t\tcd remote-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)\" \\\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)\" \\\n\t\t\trefs/heads/master)\" &&\n\t\ttest from,refs/heads/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)\" \\\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)\" \\\n\t\t\trefs/heads/master)\" &&\n\t\ttest from,refs/heads/other = \"$actual\" &&\n\t\tactual=\"$(git -c push.default=current for-each-ref \\\n\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n\t\t\trefs/heads/master)\" &&\n\t\ttest from,refs/heads/master = \"$actual\" &&\n\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n\t\t\trefs/heads/master)\" &&\n\t\ttest from,refs/heads/master = \"$actual\" &&\n\t\tactual=\"$(git -c push.default=nothing for-each-ref \\\n\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n\t\t\trefs/heads/master)\" &&\n\t\ttest from, = \"$actual\"\n\t)\n'\n\nAnd the test works with my patch.\n\nSo I decided for completude to also test with\n\t\tgit config branch.master.pushRemote to\nto test with a triangular workflow, and I found several (already existing)\nbugs.\n\nHeres what happen with such a triangular workflow when we do a `git push`:\n- with push.default=simple, we have\n\tcase PUSH_DEFAULT_SIMPLE:\n\t\tif (triangular)\n\t\t\tsetup_push_current(remote, branch);\n\t\telse\n\t\t\tsetup_push_upstream(remote, branch, triangular, 1);\n\t\tbreak;\n so the current branch is always pushed.\n- with push.default=upstream, we have\n\tcase PUSH_DEFAULT_UPSTREAM:\n\t\tsetup_push_upstream(remote, branch, triangular, 0);\n\t\tbreak;\n which then gives\n \tif (triangular)\n\t\tdie(_(\"You are pushing to remote '%s', which is not the upstream of\\n\"\n\t\t \"your current branch '%s', without telling me what to push\\n\"\n\t\t \"to update which remote branch.\"),\n\nBy the way this matches what the documentation says.\n\nHowever here is the result of\ngit -c push.default=$value for-each-ref --format=\"%(push:remotename),%(push:remoteref),%(push)\" refs/heads/master\nfor $value=\n- simple: to,,\n- upstream: to,refs/heads/other,refs/remotes/from/other\n\nNote that without my patch the %(push:remoteref) values would always be empty,\nbut my patch does not touch %(push).\n\nSo in both branch_get_push_1 and branch_get_push_remoteref I should first\ndetect if we have a triangular workflow, and update the logic of the code\naccordingly.\n"},{"id":"394884","messageId":"20200406214607.GA1251506@coredump.intra.peff.net","threadId":"52910","inReplyTo":"20200406160439.gg5uu6kepnyxpvuc@feanor","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-06T21:46:07Z","receivedAt":"2020-04-06T21:46:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 06, 2020 at 06:04:39PM +0200, Damien Robert wrote:\n\n> Heres what happen with such a triangular workflow when we do a `git push`:\n> - with push.default=simple, we have\n> \tcase PUSH_DEFAULT_SIMPLE:\n> \t\tif (triangular)\n> \t\t\tsetup_push_current(remote, branch);\n> \t\telse\n> \t\t\tsetup_push_upstream(remote, branch, triangular, 1);\n> \t\tbreak;\n> so the current branch is always pushed.\n\nYeah, otherwise every push to a remote other than origin would require a\nrefspec. I think with respect to for-each-ref, this \"triangular\" case\nwould only kick in if you define remote.pushDefault (since you can't\nspecify a remote on the command-line).\n\nThough hmm. I guess maybe it could kick in if the upstream of the branch\nis on a non-default remote? For push, that would work because\nremote_get() will look at the current branch. But of course in\nfor-each-ref, we're asking speculatively about other branches.\n\nSo I think if we want to support this triangular logic in for-each-ref,\nwe need to have a more careful definition than what's in push.c's\nis_workflow_triangular(). I.e., it would probably make sense to consider\nit from the position of \"if we were on this branch, what would it push\".\n\nAnd ditto for @{push}, I guess.\n\n> - with push.default=upstream, we have\n> \tcase PUSH_DEFAULT_UPSTREAM:\n> \t\tsetup_push_upstream(remote, branch, triangular, 0);\n> \t\tbreak;\n> which then gives\n> \tif (triangular)\n> \t\tdie(_(\"You are pushing to remote '%s', which is not the upstream of\\n\"\n> \t\t \"your current branch '%s', without telling me what to push\\n\"\n> \t\t \"to update which remote branch.\"),\n> \n> By the way this matches what the documentation says.\n\nYeah. I think in the triangular case (at least as defined in push.c)\nwe'd always be pushing to the non-upstream, so this die() makes sense.\n\nIn for-each-ref, I guess we'd hit this case with remote.pushDefault\nagain. Without that, we'd always be pushing to the upstream anyway.\n\n> However here is the result of\n> git -c push.default=$value for-each-ref --format=\"%(push:remotename),%(push:remoteref),%(push)\" refs/heads/master\n> for $value=\n> - simple: to,,\n> - upstream: to,refs/heads/other,refs/remotes/from/other\n> \n> Note that without my patch the %(push:remoteref) values would always be empty,\n> but my patch does not touch %(push).\n> \n> So in both branch_get_push_1 and branch_get_push_remoteref I should first\n> detect if we have a triangular workflow, and update the logic of the code\n> accordingly.\n\nYes, I agree this could be improved. I'm OK leaving that as a separate\nfix to your current remoteref work, though.\n\n-Peff\n"},{"id":"395593","messageId":"20200416150355.635436-1-damien.olivier.robert+git@gmail.com","threadId":"52910","inReplyTo":"20200312164558.2388589-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-16T15:03:55Z","receivedAt":"2020-04-16T15:04:30Z","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\nSigned-off-by: Damien Robert \n---\n remote.c | 94 +++++++++++++++++++++++++++++++----------\n t/t6300-for-each-ref.sh | 44 ++++++++++++++++---\n 2 files changed, 111 insertions(+), 27 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c43196ec06..352ea240cd 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@@ -1656,6 +1634,64 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\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 (branch && branch->merge && branch->merge[0] &&\n+\t\t branch->merge[0]->dst)\n+\t\t\treturn branch->merge[0]->src;\n+\t\telse\n+\t\t\treturn NULL;\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, NULL);\n+\t\t\tcur = tracking_for_push_dest(remote, branch->refname, NULL);\n+\t\t\tif (up && cur && !strcmp(cur, up))\n+\t\t\t\treturn branch->refname;\n+\t\t\telse\n+\t\t\t\treturn NULL;\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+ * 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@@ -1735,6 +1771,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..c89fd66083 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -875,12 +875,46 @@ 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\tdone\n+\t)\n+'\n+\n+test_expect_success ':push:remoteref' '\n+\tgit init push-tests &&\n+\t(\n+\t\tcd push-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)\" \\\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)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/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)\" \\\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)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/other = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=current for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=matching for-each-ref \\\n+\t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n+\t\t\trefs/heads/master)\" &&\n+\t\ttest from,refs/heads/master = \"$actual\" &&\n+\t\tactual=\"$(git -c push.default=nothing for-each-ref \\\n \t\t\t--format=\"%(push:remotename),%(push:remoteref)\" \\\n-\t\t\trefs/heads/push-simple)\" &&\n+\t\t\trefs/heads/master)\" &&\n \t\ttest from, = \"$actual\"\n \t)\n '\n-- \nPatched on top of v2.26.1-107-gefe3874640 (git version 2.26.0)\n\n"},{"id":"395594","messageId":"20200416151213.xbo5x6jt477ezwvo@feanor","threadId":"52910","inReplyTo":"20200328133134.GA1196665@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-16T15:12:13Z","receivedAt":"2020-04-16T15:14:24Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Jeff King, Sat 28 Mar 2020 at 09:31:34 (-0400) :\n> > > I am still a bit annoyed that I cannot call branch_get_push_remoteref from\n> > > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can\n> > > wait and we will need to work with the code duplication meanwhile.\n\n> > I looked into this, too, and have a working patch. It does get a little\n> > awkward, though, and I'm happy to just take your patch for now as the\n> > practical thing.\n\nHi Jeff,\n\nI looked up at your patch again, because the code duplication gets more\nannoying the more new corner cases I have to handle to get the push ref\ncorrect in all cases (cf my cover letter to v6).\n\nThis implements what I was suggesting in\nhttps://public-inbox.org/git/20200301220531.iuokzzdb5gruslrn@doriath/\n\nEssentially in branch_get_push you call:\n\n remote = remote_get(pushremote_for_branch(branch, NULL));\n tracking_for_push_dest(remote, branch_get_push_remoteref(branch),\n\nAnd as I pointed out, this is currently exactly what branch_get_push_1\ndoes, except in the\nPUSH_DEFAULT_UPSTREAM where it returns branch->merge[0]->dst.\n\nBut branch->merge is set up in `set_merge`, where we have:\n\n ret->merge[i]->src = xstrdup(ret->merge_name[i]);\n if (!remote_find_tracking(remote, ret->merge[i]) ||\n strcmp(ret->remote_name, \".\"))\n continue;\n if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),\n &oid, &ref) == 1)\n ret->merge[i]->dst = ref;\n\nSo in particular, when the remote is local, the current code path calls\ndwim_ref. (I have no idea who set up ret->merge[i]->dst if the remote is\nnot local...)\n\nSo my question was: can dwim_ref(branch->merge[0]->src) be different from\ntracking_for_push_dest(branch->merge[0]->src)?\n\nSo I admit I don't understand everything dwim_ref does, but there is at\nleast one case where the answer is yes: if we have a dangling symref,\ndwim_ref which calls expand_ref in refs.c will detect it. So in the current\ncode, %(push) would show nothing, while with your patch it would show the\ndangling symref.\n\nObviously we cannot allow a regression for this very common case ;)\n"},{"id":"395595","messageId":"20200416152145.wp2zeibxmuyas6y6@feanor","threadId":"52910","inReplyTo":"20200416150355.635436-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-04-16T15:21:45Z","receivedAt":"2020-04-16T15:21:59Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"So this patch does not solve:\n- the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/\n- the non correct result in a triangular workflow mentioned in\n https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/\n (aka v6, with an incomplete fix)\n- the code duplication between branch_get_push_remoteref and remote_get_1\n (see my answer to Jeff's code mentioned above as to why his approach\n would give a micro regression).\n\nLuckily I have figured how to solve 2) and 3). Unfortunately I haven't have\ntime to work on this since two weeks, so I have just sent the original bug\nfix first so at least this one gets fixed.\n\nThis is exactly as v4, except I moved the tests around to make them easier\nto grok.\n"},{"id":"404996","messageId":"xmqqv9gu7c61.fsf@gitster.c.googlers.com","threadId":"52910","inReplyTo":"20200416152145.wp2zeibxmuyas6y6@feanor","subject":"Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-03T22:01:10Z","receivedAt":"2020-09-03T22:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> So this patch does not solve:\n> - the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/\n> - the non correct result in a triangular workflow mentioned in\n> https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/\n> (aka v6, with an incomplete fix)\n> - the code duplication between branch_get_push_remoteref and remote_get_1\n> (see my answer to Jeff's code mentioned above as to why his approach\n> would give a micro regression).\n>\n> Luckily I have figured how to solve 2) and 3). Unfortunately I haven't have\n> time to work on this since two weeks, so I have just sent the original bug\n> fix first so at least this one gets fixed.\n>\n> This is exactly as v4, except I moved the tests around to make them easier\n> to grok.\n\nAnything new on this topic? No rush, but I'd hate to see a\nbasically good topic to be left in the stalled state too long.\n\nThanks.\n"},{"id":"405457","messageId":"20200911214358.acl3hy2e763begoo@feanor","threadId":"52910","inReplyTo":"xmqqv9gu7c61.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-09-11T21:43:58Z","receivedAt":"2020-09-11T21:44:05Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"Hi Junio,\n\nFrom Junio C Hamano, Thu 03 Sep 2020 at 15:01:10 (-0700) :\n> Anything new on this topic?\n\nUnfortunately no, work keep stacking up faster than I can unstack it...\n\n> No rush, but I'd hate to see a basically good topic to be left in the stalled state too long.\n\nHum, what about migrating the version that was in next to master? I am not\nfond of it because the series is not perfect and I am not satisfied with a\npatch series that is not as good as I would like it to be. So that was why\nI was arguing against merging it back then.\n\nOn the other hand it does correct existing bugs, and the bugs it leaves\nremaining (apart from the memory leak) happens only in exotic cases. So I\nwould not want my sense of perfection to prevent this series from graduating\ntoo long.\n\nAnd unfortunately I cannot give you an ETA for a fully satisfying series as\nI envision it.\n\nSo I guess it is your call. If you think the version that was in next is\ngood enough to graduate, I can send one last reroll with a commit message\nexplaining the remaining kinks, and iron them out later.\n\nOr more precisely, we can:\n- only use this patch (v8) without the triangular workflow fixes\n- use this patch (v8) + the triangular workflow fixes from https://public-inbox.org/git/20200406175648.25737-2-damien.olivier.robert+git@gmail.com/\n\nThe 'bug' that remains is that it detects a triangular setup, when\n1) a branch has a pushRemote but no remote and\n2) pushRemote=foobar and origin does not exists\nwhile 'git push' treat this as a non triangular workflow.\n\nIMHO 'git push' is wrong here and in my ideal perfect series it would be\nfixed there, but maybe meanwhile we can live with this small discrepancy.\n\nWhat do you think?\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"405539","messageId":"xmqqft7k0zkt.fsf@gitster.c.googlers.com","threadId":"52910","inReplyTo":"20200911214358.acl3hy2e763begoo@feanor","subject":"Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T22:21:22Z","receivedAt":"2020-09-14T22:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> Hum, what about migrating the version that was in next to master? I am not\n> fond of it because the series is not perfect and I am not satisfied with a\n> patch series that is not as good as I would like it to be. So that was why\n> I was arguing against merging it back then.\n>\n> On the other hand it does correct existing bugs, and the bugs it leaves\n> remaining (apart from the memory leak) happens only in exotic cases. So I\n> would not want my sense of perfection to prevent this series from graduating\n> too long.\n>\n> And unfortunately I cannot give you an ETA for a fully satisfying series as\n> I envision it.\n\nThat's OK---that is what \"no rush\" means.\n\nWe can throw the one bug it fixes together with the \"bugs it leaves\"\ninto the same category, i.e. happens only in narrow cases. We now\nknow that you won't be actively working on the topic right now,\nperhaps others can pick up where you left off and perhaps you can\nhelp reviewing such a follow-up work ;-)\n\nThanks.\n"}]}