From: Jeff King <peff@peff.net>
To: Damien Robert <damien.olivier.robert@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Damien Robert <damien.olivier.robert+git@gmail.com>
Subject: Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
Date: Sat, 28 Mar 2020 09:15:53 -0400 [thread overview]
Message-ID: <20200328131553.GA643242@coredump.intra.peff.net> (raw)
In-Reply-To: <20200312164558.2388589-1-damien.olivier.robert+git@gmail.com>
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
next prev parent reply other threads:[~2020-03-28 13:15 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-28 17:24 [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
2020-02-28 18:23 ` Jeff King
2020-03-01 22:05 ` Damien Robert
2020-03-02 13:32 ` Jeff King
2020-03-03 16:12 ` [PATCH v2 0/2] Damien Robert
2020-03-03 16:12 ` [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch() Damien Robert
2020-03-03 17:51 ` Junio C Hamano
2020-03-03 21:11 ` Jeff King
2020-03-03 22:22 ` Junio C Hamano
2020-03-03 16:12 ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
2020-03-03 16:29 ` Damien Robert
2020-03-03 18:29 ` Junio C Hamano
2020-03-03 18:21 ` Junio C Hamano
2020-03-03 22:24 ` Damien Robert
2020-03-03 22:48 ` Junio C Hamano
2020-03-12 16:45 ` [PATCH v3 1/1] " Damien Robert
2020-03-25 22:16 ` Damien Robert
2020-03-27 22:08 ` Junio C Hamano
2020-03-28 22:25 ` Damien Robert
2020-03-28 13:15 ` Jeff King [this message]
2020-03-28 13:31 ` Jeff King
2020-04-16 15:12 ` Damien Robert
2020-04-06 16:04 ` Damien Robert
2020-04-06 21:46 ` Jeff King
2020-04-06 17:56 ` [RFC PATCH v4 0/2] %(push) and %(push:remoteref) bug fixes Damien Robert
2020-04-06 17:56 ` [PATCH v6 1/2] remote.c: fix %(push) for triangular workflows Damien Robert
2020-04-06 17:56 ` [PATCH v6 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
2020-04-16 15:03 ` [PATCH v8 1/1] " Damien Robert
2020-04-16 15:21 ` Damien Robert
2020-09-03 22:01 ` Junio C Hamano
2020-09-11 21:43 ` Damien Robert
2020-09-14 22:21 ` Junio C Hamano
2020-03-03 16:16 ` [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
2020-03-02 13:48 ` Jeff King
2020-03-03 16:25 ` Damien Robert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200328131553.GA643242@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=damien.olivier.robert+git@gmail.com \
--cc=damien.olivier.robert@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).