| Message ID | 20200416150355.635436-1-damien.olivier.robert+git@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v8,1/1] remote.c: fix handling of %(push:remoteref) | expand |
So this patch does not solve: - the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/ - the non correct result in a triangular workflow mentioned in https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/ (aka v6, with an incomplete fix) - the code duplication between branch_get_push_remoteref and remote_get_1 (see my answer to Jeff's code mentioned above as to why his approach would give a micro regression). Luckily I have figured how to solve 2) and 3). Unfortunately I haven't have time to work on this since two weeks, so I have just sent the original bug fix first so at least this one gets fixed. This is exactly as v4, except I moved the tests around to make them easier to grok.
Damien Robert <damien.olivier.robert@gmail.com> writes: > So this patch does not solve: > - the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/ > - the non correct result in a triangular workflow mentioned in > https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/ > (aka v6, with an incomplete fix) > - the code duplication between branch_get_push_remoteref and remote_get_1 > (see my answer to Jeff's code mentioned above as to why his approach > would give a micro regression). > > Luckily I have figured how to solve 2) and 3). Unfortunately I haven't have > time to work on this since two weeks, so I have just sent the original bug > fix first so at least this one gets fixed. > > This is exactly as v4, except I moved the tests around to make them easier > to grok. Anything new on this topic? No rush, but I'd hate to see a basically good topic to be left in the stalled state too long. Thanks.
Hi Junio, From Junio C Hamano, Thu 03 Sep 2020 at 15:01:10 (-0700) : > Anything new on this topic? Unfortunately no, work keep stacking up faster than I can unstack it... > No rush, but I'd hate to see a basically good topic to be left in the stalled state too long. Hum, what about migrating the version that was in next to master? I am not fond of it because the series is not perfect and I am not satisfied with a patch series that is not as good as I would like it to be. So that was why I was arguing against merging it back then. On the other hand it does correct existing bugs, and the bugs it leaves remaining (apart from the memory leak) happens only in exotic cases. So I would not want my sense of perfection to prevent this series from graduating too long. And unfortunately I cannot give you an ETA for a fully satisfying series as I envision it. So I guess it is your call. If you think the version that was in next is good enough to graduate, I can send one last reroll with a commit message explaining the remaining kinks, and iron them out later. Or more precisely, we can: - only use this patch (v8) without the triangular workflow fixes - use this patch (v8) + the triangular workflow fixes from https://public-inbox.org/git/20200406175648.25737-2-damien.olivier.robert+git@gmail.com/ The 'bug' that remains is that it detects a triangular setup, when 1) a branch has a pushRemote but no remote and 2) pushRemote=foobar and origin does not exists while 'git push' treat this as a non triangular workflow. IMHO 'git push' is wrong here and in my ideal perfect series it would be fixed there, but maybe meanwhile we can live with this small discrepancy. What do you think?
Damien Robert <damien.olivier.robert@gmail.com> writes: > Hum, what about migrating the version that was in next to master? I am not > fond of it because the series is not perfect and I am not satisfied with a > patch series that is not as good as I would like it to be. So that was why > I was arguing against merging it back then. > > On the other hand it does correct existing bugs, and the bugs it leaves > remaining (apart from the memory leak) happens only in exotic cases. So I > would not want my sense of perfection to prevent this series from graduating > too long. > > And unfortunately I cannot give you an ETA for a fully satisfying series as > I envision it. That's OK---that is what "no rush" means. We can throw the one bug it fixes together with the "bugs it leaves" into the same category, i.e. happens only in narrow cases. We now know that you won't be actively working on the topic right now, perhaps others can pick up where you left off and perhaps you can help reviewing such a follow-up work ;-) Thanks.
diff --git a/remote.c b/remote.c index c43196ec06..352ea240cd 100644 --- a/remote.c +++ b/remote.c @@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit) return remote_for_branch(branch, explicit); } -const char *remote_ref_for_branch(struct branch *branch, int for_push) -{ - if (branch) { - if (!for_push) { - if (branch->merge_nr) { - return branch->merge_name[0]; - } - } else { - const char *dst, *remote_name = - pushremote_for_branch(branch, NULL); - struct remote *remote = remote_get(remote_name); - - if (remote && remote->push.nr && - (dst = apply_refspecs(&remote->push, - branch->refname))) { - return dst; - } - } - } - return NULL; -} - static struct remote *remote_get_1(const char *name, const char *(*get_default)(struct branch *, int *)) { @@ -1656,6 +1634,64 @@ static const char *tracking_for_push_dest(struct remote *remote, return ret; } +/** + * Return the local name of the remote tracking branch, as in + * %(push:remoteref), that corresponds to the ref we would push to given a + * bare `git push` while `branch` is checked out. + * See also branch_get_push_1 below. + */ +static const char *branch_get_push_remoteref(struct branch *branch) +{ + struct remote *remote; + + remote = remote_get(pushremote_for_branch(branch, NULL)); + if (!remote) + return NULL; + + if (remote->push.nr) { + return apply_refspecs(&remote->push, branch->refname); + } + + if (remote->mirror) + return branch->refname; + + switch (push_default) { + case PUSH_DEFAULT_NOTHING: + return NULL; + + case PUSH_DEFAULT_MATCHING: + case PUSH_DEFAULT_CURRENT: + return branch->refname; + + case PUSH_DEFAULT_UPSTREAM: + if (branch && branch->merge && branch->merge[0] && + branch->merge[0]->dst) + return branch->merge[0]->src; + else + return NULL; + + case PUSH_DEFAULT_UNSPECIFIED: + case PUSH_DEFAULT_SIMPLE: + { + const char *up, *cur; + + up = branch_get_upstream(branch, NULL); + cur = tracking_for_push_dest(remote, branch->refname, NULL); + if (up && cur && !strcmp(cur, up)) + return branch->refname; + else + return NULL; + + } + } + BUG("unhandled push situation"); +} + +/** + * Return the tracking branch, as in %(push), that corresponds to the ref we + * would push to given a bare `git push` while `branch` is checked out. + * See also branch_get_push_remoteref above. + */ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) { struct remote *remote; @@ -1735,6 +1771,20 @@ static int ignore_symref_update(const char *refname) return (flag & REF_ISSYMREF); } +const char *remote_ref_for_branch(struct branch *branch, int for_push) +{ + if (branch) { + if (!for_push) { + if (branch->merge_nr) { + return branch->merge_name[0]; + } + } else { + return branch_get_push_remoteref(branch); + } + } + return NULL; +} + /* * Create and return a list of (struct ref) consisting of copies of * each remote_ref that matches refspec. refspec must be a pattern. diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index b3c1092338..c89fd66083 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -875,12 +875,46 @@ test_expect_success ':remotename and :remoteref' ' git for-each-ref --format="${pair%=*}" \ refs/heads/master >actual && test_cmp expect actual - done && - git branch push-simple && - git config branch.push-simple.pushRemote from && - actual="$(git for-each-ref \ + done + ) +' + +test_expect_success ':push:remoteref' ' + git init push-tests && + ( + cd push-tests && + test_commit initial && + git remote add from fifth.coffee:blub && + git config branch.master.remote from && + actual="$(git -c push.default=simple for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from, = "$actual" && + git config branch.master.merge refs/heads/master && + actual="$(git -c push.default=simple for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from,refs/heads/master = "$actual" && + git config branch.master.merge refs/heads/other && + actual="$(git -c push.default=simple for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from, = "$actual" && + actual="$(git -c push.default=upstream for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from,refs/heads/other = "$actual" && + actual="$(git -c push.default=current for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from,refs/heads/master = "$actual" && + actual="$(git -c push.default=matching for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/master)" && + test from,refs/heads/master = "$actual" && + actual="$(git -c push.default=nothing for-each-ref \ --format="%(push:remotename),%(push:remoteref)" \ - refs/heads/push-simple)" && + refs/heads/master)" && test from, = "$actual" ) '
Looking at the value of %(push:remoteref) only handles the case when an explicit push refspec is passed. But it does not handle the fallback cases of looking at the configuration value of `push.default`. In particular, doing something like git config push.default current git for-each-ref --format='%(push)' git for-each-ref --format='%(push:remoteref)' prints a useful tracking ref for the first for-each-ref, but an empty string for the second. Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref: let upstream/push report the remote ref name) is to get exactly which branch `git push` will push to, even in the fallback cases, fix this. To get the meaning of %(push:remoteref), `ref-filter.c` calls `remote_ref_for_branch`. We simply add a new static helper function, `branch_get_push_remoteref` that follows the logic of `branch_get_push_1`, and call it from `remote_ref_for_branch`. We also update t/6300-for-each-ref.sh to handle all `push.default` strategies. This involves testing `push.default=simple` twice, once where there is a matching upstream branch and once when there is none. Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com> --- remote.c | 94 +++++++++++++++++++++++++++++++---------- t/t6300-for-each-ref.sh | 44 ++++++++++++++++--- 2 files changed, 111 insertions(+), 27 deletions(-)