git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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

  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).