Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

From: Jeff King <peff@peff.net>
Date: Feb 28, 2020, 18:23 UTC
Message-ID: <20200228182349.GA1408759@coredump.intra.peff.net>
In-Reply-To: <20200228172455.1734888-1-damien.olivier.robert+git@gmail.com>

On Fri, Feb 28, 2020 at 06:24:55PM +0100, Damien Robert wrote:

Show 6 quoted lines

> To get the meaning of push:remoteref, ref-filter.c calls
> remote_ref_for_branch.
> 
> However remote_ref_for_branch only handles the case of a specified refspec.
> The other cases treated by branch_get_push_1 are the mirror case,
> PUSH_DEFAULT_{NOTHING,MATCHING,CURRENT,UPSTREAM,UNSPECIFIED,SIMPLE}.

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.

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

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

If I do this:

  git init parent
  git -C parent commit --allow-empty -m foo
  
  git clone parent child
  cd child
  git branch --track mybranch origin/master
  git config push.default upstream
  git for-each-ref \
    --format='push=%(push), remoteref=%(push:remoteref)' \
    refs/heads/mybranch

the current code gives no remoteref value, which seems wrong. But with your patch I'd get "refs/heads/mybranch", which is also wrong.

I think you're right that all of the other cases would always use the same refname on the remote.

>  remote.c | 5 +++++
>  1 file changed, 5 insertions(+)

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.

Show 13 quoted lines

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

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.

-Peff