git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Damien Robert <damien.olivier.robert@gmail.com>
To: Jeff King <peff@peff.net>
Cc: J Wyman <jwyman@microsoft.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
Date: Sun, 1 Mar 2020 23:05:31 +0100	[thread overview]
Message-ID: <20200301220531.iuokzzdb5gruslrn@doriath> (raw)
In-Reply-To: <20200228182349.GA1408759@coredump.intra.peff.net>

First I apologize for sending the patch too soon, I forgot to put the RFC
flag, this is not a complete patch, and I do intend to fix the tests. I was
aware of this issue since my patch about push:track, but since I don't use
push:remoteref this was a low priority to fix. Last friday I was sick and
could not work, so I took the opportunity to scratch this itch.

From Jeff King, Fri 28 Feb 2020 at 13:23:49 (-0500) :
> Just to back up a minute to the user-visible problem, it's that:
>   git config push.default matching
>   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. That feature (and remote_ref_for_branch) come
> from 9700fae5ee (for-each-ref: let upstream/push report the remote ref
> name, 2017-11-07). Author cc'd for guidance.

Yes exactly.
 
> I wonder if %(upstream:remoteref) has similar problems, but I suppose
> not (it doesn't have this implicit config, so we'd always either have a
> remote ref or we'd have no upstream at all).

And the code is different. upstream:remoteref uses branch->merge_name[0].
This is due to the fact that the branch struct stores different things for
merge branches than for push branches (which hurt my sense of symmetry :)).

> > In all these cases, either there is no push remote, or the remote_ref is
> > branch->refname. So we can handle all these cases by returning
> > branch->refname, provided that remote is not empty.
> In the case of "upstream", the names could be different, couldn't they?

Yes of course. Moreover there is also the case of 'nothing' where we should
not return the branch name (so what we should test for in the other cases
is not the existence of `remote` but of `branch->push_remote_ref`.)

> We'd want some test coverage to make sure this doesn't regress. There
> are already some tests covering this feature in t6300. And indeed, your
> patch causes them to fail when checking a "simple" push case (but I
> think I'd argue the current expected value there is wrong). That should
> be expanded to cover the "upstream" case, too, once we figure out how to
> get it right.

Yes, I'll do both simple and upstream for tests I think.

> > diff --git a/remote.c b/remote.c
> > index 593ce297ed..75e42b1e36 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
> >  					*explicit = 1;
> >  				return dst;
> >  			}
> > +			else if (remote) {
> > +				if (explicit)
> > +					*explicit = 1;
> > +				return branch->refname;
> > +			}
> 
> Saying "*explicit = 1" here seems weird. Isn't the whole point that
> these modes _aren't_ explicit?

Well pushremote_for_branch also set explicit=1 if only remote.pushDefault
is set, so I followed suit.

> It looks like our only caller will ignore our return value unless we say
> "explicit", though. I have to wonder what the point of that flag is,
> versus just returning NULL when we don't have anything to return.

I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the
situation is handled by RR_REMOTE_REF, where explicit is not used at all.
So we could remove it.


So it remains the problem of handling the 'upstream' case.
The ideal solution would be to not duplicate branch_get_push_1.
In most of the case, this function finds `dst` which is exactly the
push:remoteref we are looking for. 

Then branch_get_push_1 uses
		ret = tracking_for_push_dest(remote, dst, err);
which simply calls
	ret = apply_refspecs(&remote->fetch, dst);

The only different case is
	case PUSH_DEFAULT_UPSTREAM:
		return branch_get_upstream(branch, err);
which returns
	branch->merge[0]->dst

So we could almost write an auxiliary function that returns push:remoteref
and use it both in remote_ref_for_branch and branch_get_push_1 (via a
further call to tracking_for_push_dest) except for the 'upstream' case
which is subtly different.

In the 'upstream' case, the auxiliary function would return
branch->merge_name[0]. So the question is: can
tracking_for_push_dest(branch->merge_name[0]) be different from
branch->merge[0]->dst?

Now branch->merge is set in `set_merge`, where it is constructed via
		if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
			     &oid, &ref) == 1)
And I don't understand dwim_ref enough to know if there could be
differences in our setting from tracking_for_push_dest in corner cases.


Another solution could be as follow: we already store `push` in
`branch->push_tracking_ref`. So the question is: can we always easily convert
something like refs/remotes/origin/branch_name to refs/heads/branch_name
(ie essentially reverse ètracking_for_push_dest`), or are there corner cases?


Otherwise a simple but not elegant solution would be to copy paste the
code of branch_get_push_1 to remote_ref_for_branch, simply removing the
calls to `tracking_for_push_dest` and using remote->branch_name[0] rather
than remote->branch[0]->dst for the upstream case.



-- 
Damien Robert
http://www.normalesup.org/~robert/pro

  reply	other threads:[~2020-03-01 22:05 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-28 17:24 Damien Robert
2020-02-28 18:23 ` Jeff King
2020-03-01 22:05   ` Damien Robert [this message]
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
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=20200301220531.iuokzzdb5gruslrn@doriath \
    --to=damien.olivier.robert@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jwyman@microsoft.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref' \
    /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

Code repositories for project(s) associated with this 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).