| Message ID | 20200312164558.2388589-1-damien.olivier.robert+git@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/1] remote.c: fix handling of %(push:remoteref) | expand |
Hi Junio, IMHO this patch should be good to cook. Thanks!
Damien Robert <damien.olivier.robert@gmail.com> writes:
> IMHO this patch should be good to cook.
Would love to queue it but I haven't had a time to look at it.
Thanks.
On Thu, Mar 12, 2020 at 05:45:58PM +0100, Damien Robert wrote: > 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`. I looked at this again with fresh eyes, and I think it's a pretty practical fix all around. I noticed one memory leak, but it's actually there already. :-/ > I ended up following most of Junio's suggestion, except having a > default: BUG(...) > and returning NULL at the end of the case. > > I prefer to return explicitly in each case statement rather than use break > to fallback at the end of the case. What you have looks reasonable to me (and would hopefully get us a compiler warning if new push modes are added). > I said I would also update branch_get_push1 to be as similar as possible to > branch_get_push_remoteref, but because of the error handling of the latter, > it would makes the syntax a bit weird, so I did not touch it. > > I am still a bit annoyed that I cannot call branch_get_push_remoteref from > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can > wait and we will need to work with the code duplication meanwhile. I looked into this, too, and have a working patch. It does get a little awkward, though, and I'm happy to just take your patch for now as the practical thing. > -const char *remote_ref_for_branch(struct branch *branch, int for_push) > [...] > - 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; > - } This is the leak in the old code. apply_refspecs() returns a newly allocated buffer, but our caller would never know to free it because we return a const pointer. And we have the same problem in the new code: > +static const char *branch_get_push_remoteref(struct branch *branch) > [...] > + if (remote->push.nr) { > + return apply_refspecs(&remote->push, branch->refname); > + } But we can't return a "char *", because all of the other return values point to long-lived strings that the caller won't own. One way to solve it would be to xstrdup() all of those. I'm not thrilled about that, though; for-each-ref already does way more allocations-per-ref than I'd like. A better solution would be for this function to write the result into a strbuf. For one-off calls that's no worse than allocating a string to return, and for repeated calls like for-each-ref, it could reuse the same allocated strbuf over and over. Since this leak existed before your patch, I'm inclined to treat it as a separate topic and not have it hold up this fix. > +static const char *branch_get_push_remoteref(struct branch *branch) > [...] All the logic here makes sense to me. > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh The tests makes sense to me, though I found a few nits to pick: > index 9c910ce746..60e21834fd 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' ' > actual="$(git for-each-ref \ > --format="%(push:remotename),%(push:remoteref)" \ > refs/heads/push-simple)" && > - test from, = "$actual" > + test from, = "$actual" && The existing tests just assume taht push.default=simple. Since we're now testing everything, should this be "git -c push.default=simple" to be more explicit? Likewise, is it worth labeling all of the simple cases with a comment (or possibly putting them in separate tests, though I guess some of the setup is shared)? This one expects blank because there's no configured upstream. > + git config branch.push-simple.remote from && > + git config branch.push-simple.merge refs/heads/master && > + actual="$(git for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from, = "$actual" && This one expects blank because the upstream and local names don't match. > + actual="$(git -c push.default=upstream for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from,refs/heads/master = "$actual" && This one has a real configured upstream (and relies on the branch.*.merge config set above). OK. It's a little funny that the branch is still called push-simple. :) > + actual="$(git -c push.default=current for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from,refs/heads/push-simple = "$actual" && Same name on the other side. OK. > + actual="$(git -c push.default=matching for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from,refs/heads/push-simple = "$actual" && Ditto for matching, which I think is the only sensible output. > + actual="$(git -c push.default=nothing for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from, = "$actual" && Nothing for nothing. Makes sense. > + git config branch.push-simple.merge refs/heads/push-simple && > + actual="$(git for-each-ref \ > + --format="%(push:remotename),%(push:remoteref)" \ > + refs/heads/push-simple)" && > + test from,refs/heads/push-simple = "$actual" And this is a real simple that actually shows something. It would make more sense to me with the other "simple" tests, but I guess _not_ having the upstream set to the same name is important for the quality of the "current" and "upstream" tests. Maybe we could do this test first, before setting branch.*.merge to refs/heads/master? -Peff
On Sat, Mar 28, 2020 at 09:15:53AM -0400, Jeff King wrote: > > I said I would also update branch_get_push1 to be as similar as possible to > > branch_get_push_remoteref, but because of the error handling of the latter, > > it would makes the syntax a bit weird, so I did not touch it. > > > > I am still a bit annoyed that I cannot call branch_get_push_remoteref from > > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can > > wait and we will need to work with the code duplication meanwhile. > > I looked into this, too, and have a working patch. It does get a little > awkward, though, and I'm happy to just take your patch for now as the > practical thing. Here's what I came up with (against master, but I stole a few bits from your patch to connect it to remote_ref_for_branch and test it). I'll quote bits of it to comment inline, and you can find the complete patch at the bottom. > @@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch, > } > > __attribute__((format (printf,2,3))) > -static const char *error_buf(struct strbuf *err, const char *fmt, ...) > +static void *error_buf(struct strbuf *err, const char *fmt, ...) > { > if (err) { > va_list ap; This loosens up error_buf() to make it usable for functions that aren't returning a string. Which we use for... > @@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...) > return NULL; > } > > -const char *branch_get_upstream(struct branch *branch, struct strbuf *err) > +struct refspec_item *branch_get_upstream_refspec(struct branch *branch, > + struct strbuf *err) > { > if (!branch) > return error_buf(err, _("HEAD does not point to a branch")); > @@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err) > _("upstream branch '%s' not stored as a remote-tracking branch"), > branch->merge[0]->src); > > - return branch->merge[0]->dst; > + return branch->merge[0]; > +} > + > +const char *branch_get_upstream(struct branch *branch, struct strbuf *err) > +{ > + struct refspec_item *ret = branch_get_upstream_refspec(branch, err); > + if (ret) > + return ret->dst; > + return NULL; > } > We can't use branch_get_upstream() to get the remote side, since it returns branch->merge[0]->dst, and we'd want branch->merge[0]->src. So this factors out a helper that returns both sides, and branch_get_upstream() can pick out "dst". > @@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote, > return ret; > } > > -static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) > +static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err) > { > struct remote *remote; > Here I was able to just convert push_1 into push_remoteref, since it has all of the error-handling we want (and the error_buf() helper makes it safe to pass a NULL and just ignore the errors if a caller wants to). And then push_1 essentially becomes: const char *remoteref = branch_get_push_remoteref(branch, err); return tracking_for_push_dest(remote, remoteref, err); > @@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) > branch->name); > > if (remote->push.nr) { > - char *dst; > - const char *ret; > - > - dst = apply_refspecs(&remote->push, branch->refname); > + char *dst = apply_refspecs(&remote->push, branch->refname); > if (!dst) > return error_buf(err, > _("push refspecs for '%s' do not include '%s'"), > remote->name, branch->name); > > - ret = tracking_for_push_dest(remote, dst, err); > - free(dst); > - return ret; > + return dst; > } We're really just dropping the tracking_for_push_dest() here, since we want the remote side. This matches what your patch did, except that we have the error handling. By the way, this is how I noticed the memory leak. And this patch does make it worse because we used to get the cleanup of "dst" right, but now it will be split across two functions and we won't free it. For that matter, tracking_for_push_dest() also returns an allocated string as a "const char *", so it's a leak, too. Maybe the strbuf plan is worth pursuing. :) > if (remote->mirror) > - return tracking_for_push_dest(remote, branch->refname, err); > + return branch->refname; Again, just dropping the tracking conversion, and it matches your patch. > switch (push_default) { > case PUSH_DEFAULT_NOTHING: > return error_buf(err, _("push has no destination (push.default is 'nothing')")); We don't need to touch anything here, because both the remote and local sides are NULL. :) > > case PUSH_DEFAULT_MATCHING: > case PUSH_DEFAULT_CURRENT: > - return tracking_for_push_dest(remote, branch->refname, err); > + return branch->refname; Again, just dropping tracking. > case PUSH_DEFAULT_UPSTREAM: > - return branch_get_upstream(branch, err); > + { > + struct refspec_item *ret = > + branch_get_upstream_refspec(branch, err); > + if (ret) > + return ret->src; > + return NULL; > + } Here we have to use the new helper to pull out the "src" side. This unfortunately means that to get the tracking ref, we'll re-apply tracking_for_push_dest(), even though we could have just gotten the actual value we wanted from ret->dst (without a memory leak!). > @@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) > if (strcmp(cur, up)) > return error_buf(err, > _("cannot resolve 'simple' push to a single destination")); > - return cur; > + return branch->refname; > } And here again we're less efficient. We already checked the tracking here to make sure we have the same name on both sides. But when we return, we'll still apply tracking_for_push_dest(), which will just give us the same name again (but the caller doesn't know that). > @@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err) > if (!branch) > return error_buf(err, _("HEAD does not point to a branch")); > > - if (!branch->push_tracking_ref) > - branch->push_tracking_ref = branch_get_push_1(branch, err); > + if (!branch->push_tracking_ref) { > + const char *remoteref = branch_get_push_remoteref(branch, err); > + if (remoteref) { > + /* > + * ugh, we have to find remote again; should there be a > + * master function which returns both remote and remoteref? > + */ > + struct remote *remote = > + remote_get(pushremote_for_branch(branch, NULL)); > + branch->push_tracking_ref = > + tracking_for_push_dest(remote, remoteref, err); > + } > + } > return branch->push_tracking_ref; And here I just dropped push_1 entirely and did it inline. The extra remote is ugly though. I guess we could return it as an out-parameter. So it does work, but it's kind of awkward. And even if we solved the leaking problem by using a strbuf, that would make it doubly awkward, because the code above would need an _extra_ strbuf to store the branch_get_push_remoteref() result, and only to then convert it via tracking_for_push_dest(). Full patch is below if you want to try it out or hack on it further. --- diff --git a/remote.c b/remote.c index c43196ec06..22144c96b5 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 *)) { @@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch, } __attribute__((format (printf,2,3))) -static const char *error_buf(struct strbuf *err, const char *fmt, ...) +static void *error_buf(struct strbuf *err, const char *fmt, ...) { if (err) { va_list ap; @@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...) return NULL; } -const char *branch_get_upstream(struct branch *branch, struct strbuf *err) +struct refspec_item *branch_get_upstream_refspec(struct branch *branch, + struct strbuf *err) { if (!branch) return error_buf(err, _("HEAD does not point to a branch")); @@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err) _("upstream branch '%s' not stored as a remote-tracking branch"), branch->merge[0]->src); - return branch->merge[0]->dst; + return branch->merge[0]; +} + +const char *branch_get_upstream(struct branch *branch, struct strbuf *err) +{ + struct refspec_item *ret = branch_get_upstream_refspec(branch, err); + if (ret) + return ret->dst; + return NULL; } static const char *tracking_for_push_dest(struct remote *remote, @@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote, return ret; } -static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) +static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err) { struct remote *remote; @@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) branch->name); if (remote->push.nr) { - char *dst; - const char *ret; - - dst = apply_refspecs(&remote->push, branch->refname); + char *dst = apply_refspecs(&remote->push, branch->refname); if (!dst) return error_buf(err, _("push refspecs for '%s' do not include '%s'"), remote->name, branch->name); - ret = tracking_for_push_dest(remote, dst, err); - free(dst); - return ret; + return dst; } if (remote->mirror) - return tracking_for_push_dest(remote, branch->refname, err); + return branch->refname; switch (push_default) { case PUSH_DEFAULT_NOTHING: return error_buf(err, _("push has no destination (push.default is 'nothing')")); case PUSH_DEFAULT_MATCHING: case PUSH_DEFAULT_CURRENT: - return tracking_for_push_dest(remote, branch->refname, err); + return branch->refname; case PUSH_DEFAULT_UPSTREAM: - return branch_get_upstream(branch, err); + { + struct refspec_item *ret = + branch_get_upstream_refspec(branch, err); + if (ret) + return ret->src; + return NULL; + } case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: @@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) if (strcmp(cur, up)) return error_buf(err, _("cannot resolve 'simple' push to a single destination")); - return cur; + return branch->refname; } } @@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err) if (!branch) return error_buf(err, _("HEAD does not point to a branch")); - if (!branch->push_tracking_ref) - branch->push_tracking_ref = branch_get_push_1(branch, err); + if (!branch->push_tracking_ref) { + const char *remoteref = branch_get_push_remoteref(branch, err); + if (remoteref) { + /* + * ugh, we have to find remote again; should there be a + * master function which returns both remote and remoteref? + */ + struct remote *remote = + remote_get(pushremote_for_branch(branch, NULL)); + branch->push_tracking_ref = + tracking_for_push_dest(remote, remoteref, err); + } + } return branch->push_tracking_ref; } @@ -1735,6 +1734,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, NULL); + } + } + 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 9c910ce746..60e21834fd 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' ' actual="$(git for-each-ref \ --format="%(push:remotename),%(push:remoteref)" \ refs/heads/push-simple)" && - test from, = "$actual" + test from, = "$actual" && + git config branch.push-simple.remote from && + git config branch.push-simple.merge refs/heads/master && + actual="$(git for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from, = "$actual" && + actual="$(git -c push.default=upstream for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/master = "$actual" && + actual="$(git -c push.default=current for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$actual" && + actual="$(git -c push.default=matching for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$actual" && + actual="$(git -c push.default=nothing for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from, = "$actual" && + git config branch.push-simple.merge refs/heads/push-simple && + actual="$(git for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$actual" ) '
From Junio C Hamano, Fri 27 Mar 2020 at 15:08:14 (-0700) : > Damien Robert <damien.olivier.robert@gmail.com> writes: > > IMHO this patch should be good to cook. > Would love to queue it but I haven't had a time to look at it. Sure, no worries, I was just wondering if this was on your todo list or if the last patch version fell through the cracks (I have no idea how you manage to keep up with the traffic of this ml). Your message managed to invoke Jeff who suggests some improvements to the test, so I'll reroll anyway :)
From Jeff King, Sat 28 Mar 2020 at 09:15:53 (-0400) :
> The tests makes sense to me, though I found a few nits to pick:
[...]
So I updated the tests as follow. I put them in another test, as you
suggested, this is indeed much clearer.
test_expect_success ':push:remoteref' '
git init remote-tests &&
(
cd remote-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/master)" &&
test from, = "$actual"
)
'
And the test works with my patch.
So I decided for completude to also test with
git config branch.master.pushRemote to
to test with a triangular workflow, and I found several (already existing)
bugs.
Heres what happen with such a triangular workflow when we do a `git push`:
- with push.default=simple, we have
case PUSH_DEFAULT_SIMPLE:
if (triangular)
setup_push_current(remote, branch);
else
setup_push_upstream(remote, branch, triangular, 1);
break;
so the current branch is always pushed.
- with push.default=upstream, we have
case PUSH_DEFAULT_UPSTREAM:
setup_push_upstream(remote, branch, triangular, 0);
break;
which then gives
if (triangular)
die(_("You are pushing to remote '%s', which is not the upstream of\n"
"your current branch '%s', without telling me what to push\n"
"to update which remote branch."),
By the way this matches what the documentation says.
However here is the result of
git -c push.default=$value for-each-ref --format="%(push:remotename),%(push:remoteref),%(push)" refs/heads/master
for $value=
- simple: to,,
- upstream: to,refs/heads/other,refs/remotes/from/other
Note that without my patch the %(push:remoteref) values would always be empty,
but my patch does not touch %(push).
So in both branch_get_push_1 and branch_get_push_remoteref I should first
detect if we have a triangular workflow, and update the logic of the code
accordingly.
On Mon, Apr 06, 2020 at 06:04:39PM +0200, Damien Robert wrote: > Heres what happen with such a triangular workflow when we do a `git push`: > - with push.default=simple, we have > case PUSH_DEFAULT_SIMPLE: > if (triangular) > setup_push_current(remote, branch); > else > setup_push_upstream(remote, branch, triangular, 1); > break; > so the current branch is always pushed. Yeah, otherwise every push to a remote other than origin would require a refspec. I think with respect to for-each-ref, this "triangular" case would only kick in if you define remote.pushDefault (since you can't specify a remote on the command-line). Though hmm. I guess maybe it could kick in if the upstream of the branch is on a non-default remote? For push, that would work because remote_get() will look at the current branch. But of course in for-each-ref, we're asking speculatively about other branches. So I think if we want to support this triangular logic in for-each-ref, we need to have a more careful definition than what's in push.c's is_workflow_triangular(). I.e., it would probably make sense to consider it from the position of "if we were on this branch, what would it push". And ditto for @{push}, I guess. > - with push.default=upstream, we have > case PUSH_DEFAULT_UPSTREAM: > setup_push_upstream(remote, branch, triangular, 0); > break; > which then gives > if (triangular) > die(_("You are pushing to remote '%s', which is not the upstream of\n" > "your current branch '%s', without telling me what to push\n" > "to update which remote branch."), > > By the way this matches what the documentation says. Yeah. I think in the triangular case (at least as defined in push.c) we'd always be pushing to the non-upstream, so this die() makes sense. In for-each-ref, I guess we'd hit this case with remote.pushDefault again. Without that, we'd always be pushing to the upstream anyway. > However here is the result of > git -c push.default=$value for-each-ref --format="%(push:remotename),%(push:remoteref),%(push)" refs/heads/master > for $value= > - simple: to,, > - upstream: to,refs/heads/other,refs/remotes/from/other > > Note that without my patch the %(push:remoteref) values would always be empty, > but my patch does not touch %(push). > > So in both branch_get_push_1 and branch_get_push_remoteref I should first > detect if we have a triangular workflow, and update the logic of the code > accordingly. Yes, I agree this could be improved. I'm OK leaving that as a separate fix to your current remoteref work, though. -Peff
From Jeff King, Sat 28 Mar 2020 at 09:31:34 (-0400) : > > > I am still a bit annoyed that I cannot call branch_get_push_remoteref from > > > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can > > > wait and we will need to work with the code duplication meanwhile. > > I looked into this, too, and have a working patch. It does get a little > > awkward, though, and I'm happy to just take your patch for now as the > > practical thing. Hi Jeff, I looked up at your patch again, because the code duplication gets more annoying the more new corner cases I have to handle to get the push ref correct in all cases (cf my cover letter to v6). This implements what I was suggesting in https://public-inbox.org/git/20200301220531.iuokzzdb5gruslrn@doriath/ Essentially in branch_get_push you call: remote = remote_get(pushremote_for_branch(branch, NULL)); tracking_for_push_dest(remote, branch_get_push_remoteref(branch), And as I pointed out, this is currently exactly what branch_get_push_1 does, except in the PUSH_DEFAULT_UPSTREAM where it returns branch->merge[0]->dst. But branch->merge is set up in `set_merge`, where we have: ret->merge[i]->src = xstrdup(ret->merge_name[i]); if (!remote_find_tracking(remote, ret->merge[i]) || strcmp(ret->remote_name, ".")) continue; if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]), &oid, &ref) == 1) ret->merge[i]->dst = ref; So in particular, when the remote is local, the current code path calls dwim_ref. (I have no idea who set up ret->merge[i]->dst if the remote is not local...) So my question was: can dwim_ref(branch->merge[0]->src) be different from tracking_for_push_dest(branch->merge[0]->src)? So I admit I don't understand everything dwim_ref does, but there is at least one case where the answer is yes: if we have a dangling symref, dwim_ref which calls expand_ref in refs.c will detect it. So in the current code, %(push) would show nothing, while with your patch it would show the dangling symref. Obviously we cannot allow a regression for this very common case ;)
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 9c910ce746..60e21834fd 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' ' actual="$(git for-each-ref \ --format="%(push:remotename),%(push:remoteref)" \ refs/heads/push-simple)" && - test from, = "$actual" + test from, = "$actual" && + git config branch.push-simple.remote from && + git config branch.push-simple.merge refs/heads/master && + actual="$(git for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from, = "$actual" && + actual="$(git -c push.default=upstream for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/master = "$actual" && + actual="$(git -c push.default=current for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$actual" && + actual="$(git -c push.default=matching for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$actual" && + actual="$(git -c push.default=nothing for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from, = "$actual" && + git config branch.push-simple.merge refs/heads/push-simple && + actual="$(git for-each-ref \ + --format="%(push:remotename),%(push:remoteref)" \ + refs/heads/push-simple)" && + test from,refs/heads/push-simple = "$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> --- I ended up following most of Junio's suggestion, except having a default: BUG(...) and returning NULL at the end of the case. I prefer to return explicitly in each case statement rather than use break to fallback at the end of the case. I said I would also update branch_get_push1 to be as similar as possible to branch_get_push_remoteref, but because of the error handling of the latter, it would makes the syntax a bit weird, so I did not touch it. I am still a bit annoyed that I cannot call branch_get_push_remoteref from branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can wait and we will need to work with the code duplication meanwhile. remote.c | 94 +++++++++++++++++++++++++++++++---------- t/t6300-for-each-ref.sh | 29 ++++++++++++- 2 files changed, 100 insertions(+), 23 deletions(-)