Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes

"Harald Nordgren via GitGitGadget" <[email protected]> writes:

> From: Harald Nordgren <[email protected]>
>
> A branch may name its push destination with a URL instead of a
> configured remote. This is useful in fork workflows, where the original
> remote is renamed to "upstream", the fork is added as "origin", and an
> existing branch.<name>.pushRemote continues to contain the fork URL.
>
> Git can still push through the anonymous remote created for that URL.
> However, the anonymous remote has no fetch refspec. Git therefore cannot
> resolve @{push} to origin/<branch> or update that remote-tracking branch
> after a push. The push can succeed, or report that everything is up to
> date, while status continues to compare against a stale tracking ref or
> cannot show the push branch at all.
>
> A uniquely matching configured remote already provides the missing
> mapping. Use its fetch refspec when resolving the push tracking branch
> and when updating tracking refs after a push. This changes neither the
> push destination nor configuration. Keep the existing behavior when no
> remote matches or multiple remotes share the URL, since either case is
> ambiguous.
> ...
> +struct remote *repo_remote_for_push_tracking(struct repository *repo,
> +					     struct remote *remote)
> +{
> +	struct remote *first_match = NULL;
> +	struct remote_state *remote_state = repo->remote_state;
> +
> +	if (remote->origin != REMOTE_UNCONFIGURED || remote->url.nr != 1)
> +		return remote;

I briefly wondered what should happen when a caller passes NULL as
the remote parameter to this function, but it turns out that no
caller passes NULL.  One caller is tracking_for_push_dest(),
which is called from branch_get_push_1().  The latter refuses to
proceed when !remote is true and does not call
tracking_for_push_dest(), meaning it cannot pass NULL to this
function.  The other caller is transport_push(), which passes
transport->remote.  This value comes from transport_get(), which
ensures transport->remote is not NULL before returning, so it
cannot pass NULL to this function either.

Therefore, it is OK to assume remote is not NULL, and let the
program crash loudly if that assumption is violated.  Adding an
explicit BUG() check would be overkill here:

    if (!repo || !remote)
            BUG("...");

> +	for (int i = 0; i < remote_state->remotes_nr; i++) {
> +		struct remote *candidate = remote_state->remotes[i];
> +
> +		if (!candidate || candidate == remote ||
> +		    !remote_is_configured(candidate, 0) ||
> +		    !remote_has_url(candidate, remote->url.v[0]))
> +			continue;

This check, as well as the safety uniqueness check at the beginning
of the function, only pays attention to the url member.  However, it
should also consider the pushurl member and, when it exists, ignore
the url member.  The upfront check would then look something like
this (please sanity check the details):

	const char *check_url = NULL;

	if (remote->origin != REMOTE_UNCONFIGURED)
		return remote;

	if (remote->pushurl.nr) {
		if (remote->pushurl.nr != 1)
			return remote;
		check_url = remote->pushurl.v[0];
	} else if (remote->url.nr != 1) {
		return remote;
	} else {
		check_url = remote->url.v[0];
	}

The test inside the loop would then use check_url:

		!remote_has_url(candidate, check_url)

instead of testing remote->url.v[0] directly.

Thanks.