git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Damien Robert <damien.olivier.robert@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Damien Robert <damien.olivier.robert+git@gmail.com>
Subject: Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
Date: Tue, 03 Mar 2020 10:21:31 -0800	[thread overview]
Message-ID: <xmqqtv358fkk.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20200303161223.1870298-3-damien.olivier.robert+git@gmail.com> (Damien Robert's message of "Tue, 3 Mar 2020 17:12:23 +0100")

Damien Robert <damien.olivier.robert@gmail.com> writes:

> Looking at the value of %(push:remoteref) only handles the case when an
> explicit push refspec is passed. But it does not handle the fallback
> cases of looking at the configuration value of `push.default`.
>
> In particular, doing something like
>
>     git config push.default current
>     git for-each-ref --format='%(push)'
>     git for-each-ref --format='%(push:remoteref)'
>
> prints a useful tracking ref for the first for-each-ref, but an empty
> string for the second.
>
> 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`.
>
> We also update t/6300-for-each-ref.sh to handle all `push.default`
> strategies. This involves testing `push.default=simple` twice, once
> where there is a matching upstream branch and once when there is none.
>
> Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
> ---
>  remote.c                | 106 +++++++++++++++++++++++++++++++---------
>  t/t6300-for-each-ref.sh |  29 ++++++++++-
>  2 files changed, 112 insertions(+), 23 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index c43196ec06..b3ce992d01 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
>  	return remote_for_branch(branch, explicit);
>  }
>  
> -const char *remote_ref_for_branch(struct branch *branch, int for_push)
> -{
> -	if (branch) {
> -		if (!for_push) {
> -			if (branch->merge_nr) {
> -				return branch->merge_name[0];
> -			}
> -		} else {
> -			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;
> -			}
> -		}
> -	}
> -	return NULL;
> -}

Mental note: this function was moved down, the main part of the
logic extracted to a new branch_get_push_remoteref() helper, which
in addition got extended.

> @@ -1656,6 +1634,76 @@ static const char *tracking_for_push_dest(struct remote *remote,
>  	return ret;
>  }
>  
> +/**
> + * Return the local name of the remote tracking branch, as in
> + * %(push:remoteref), that corresponds to the ref we would push to given a
> + * bare `git push` while `branch` is checked out.
> + * See also branch_get_push_1 below.
> + */
> +static const char *branch_get_push_remoteref(struct branch *branch)
> +{
> +	struct remote *remote;
> +
> +	remote = remote_get(pushremote_for_branch(branch, NULL));
> +	if (!remote)
> +		return NULL;
> +
> +	if (remote->push.nr) {
> +		char *dst;
> +
> +		dst = apply_refspecs(&remote->push, branch->refname);
> +		if (!dst)
> +			return NULL;
> +
> +		return dst;
> +	}

That's a fairly expensive way to write

	if (remote->push.nr)
		return apply_refspecs(&remote->push, branch->refname);

one-liner.

In any case, up to this point, the code does exactly the same thing
as the original (i.e. when remote.<remotename>.push is set and
covers the current branch, use that to figure out which way we are
pushing).

> +	if (remote->mirror)
> +		return branch->refname;

If mirroring, we push to the same name, OK.

> +	switch (push_default) {
> +	case PUSH_DEFAULT_NOTHING:
> +		return NULL;
> +
> +	case PUSH_DEFAULT_MATCHING:
> +	case PUSH_DEFAULT_CURRENT:
> +		return branch->refname;

These three cases are straight-forward, I think.

> +	case PUSH_DEFAULT_UPSTREAM:
> +		{
> +			if (!branch || !branch->merge ||
> +			    !branch->merge[0] || !branch->merge[0]->dst)
> +			return NULL;
> +
> +			return branch->merge[0]->src;
> +		}

This is strangely indented and somewhat unreadable.  Why isn't this
more like:

	case PUSH_DEFAULT_UPSTREAM:
		if (branch && branch->merge && branch->merge[0] &&
		    branch->merge[0]->dst)
			return branch->merge[0]->src;
		break;

and have "return NULL" after the switch() statement before we leave
the function?

> +
> +	case PUSH_DEFAULT_UNSPECIFIED:
> +	case PUSH_DEFAULT_SIMPLE:
> +		{
> +			const char *up, *cur;
> +
> +			up = branch_get_upstream(branch, NULL);
> +			if (!up)
> +				return NULL;
> +			cur = tracking_for_push_dest(remote, branch->refname, NULL);
> +			if (!cur)
> +				return NULL;
> +			if (strcmp(cur, up))
> +				return NULL;

This is probably not all that performance critical, so

			up = branch_get_upstream(branch, NULL);
			current = tracking_for_push_dest(remote, branch->refname, NULL);
			if (!up || !current || strcmp(current, up))
				return NULL;

might be easier to follow.

> +			return branch->refname;

> +		}
> +	}
> +
> +	BUG("unhandled push situation");

This is better done / easier to read inside switch() as default:
clause.

By the way, I have a bit higher-level question.  

All of the above logic that decides what should happen in "git push"
MUST have existing code we already use to implement "git push", no?

Why do we need to reinvent it here, instead of reusing the existing
code?  Is it because the interface into the functions that implement
the existing logic is very different from what this function wants?


  parent reply	other threads:[~2020-03-03 18:21 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 [this message]
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
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=xmqqtv358fkk.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=damien.olivier.robert+git@gmail.com \
    --cc=damien.olivier.robert@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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).