Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

From: Jeff King <peff@peff.net>
Date: Mar 28, 2020, 13:15 UTC
Message-ID: <20200328131553.GA643242@coredump.intra.peff.net>
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:

Show 8 quoted lines

> 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. :-/

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 11 quoted lines

> -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:

Show 5 quoted lines

> +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:

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

> +		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