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
