git@vger.kernel.org list mirror (unofficial, 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 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
Date: Tue, 03 Mar 2020 09:51:07 -0800	[thread overview]
Message-ID: <xmqqzhcx8gz8.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20200303161223.1870298-2-damien.olivier.robert+git@gmail.com> (Damien Robert's message of "Tue, 3 Mar 2020 17:12:22 +0100")

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

> From: Jeff King <peff@peff.net>
>
> Commit 9700fae5ee (for-each-ref: let upstream/push report the remote ref
> name, 2017-11-07) added a remote_ref_for_branch() helper, which is
> modeled after remote_for_branch(). This includes providing an "explicit"
> out-parameter that tells the caller whether the remote was configured by
> the user, or whether we picked a default name like "origin".
>
> But unlike remote names, there's no default case for the remote branch
> name.

Up to this point, it is well written and easy to read.  I think
"there is no case where a default name for the remoate branch is
used" would be even more easy to read.

In any case, if there is no case that default name, I understand
that explicit is always set to 1?

> In any case where we don't set "explicit", we'd just an empty
> string anyway.

Sorry, but I cannot parse this.  But earlier, you established that
there is no case that a default is used, so is there any case where
we don't set "explicit"?  I don't get it.

> Let's instead return NULL in this case, letting us
> simplify the function interface.

>  	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
> -		int explicit;
>  		const char *merge;
>  
> -		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
> -					      &explicit);
> -		*s = xstrdup(explicit ? merge : "");
> +		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push);
> +		*s = xstrdup(merge ? merge : "");

Mental note: as long as explicit is set to true when the function
returns a non-null value, this change will be a no-op as far as the
result is concerned, which is a good thing.

> diff --git a/remote.c b/remote.c
> index 593ce297ed..c43196ec06 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -516,14 +516,11 @@ 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,
> -				  int *explicit)
> +const char *remote_ref_for_branch(struct branch *branch, int for_push)
>  {
>  	if (branch) {
>  		if (!for_push) {
>  			if (branch->merge_nr) {
> -				if (explicit)
> -					*explicit = 1;
>  				return branch->merge_name[0];

This case returns a non-NULL pointer, and used to set explicit to
true.

>  			}
>  		} else {
> @@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
>  			if (remote && remote->push.nr &&
>  			    (dst = apply_refspecs(&remote->push,
>  						  branch->refname))) {
> -				if (explicit)
> -					*explicit = 1;
>  				return dst;

So did this.

>  			}
>  		}
>  	}
> -	if (explicit)
> -		*explicit = 0;
> -	return "";
> +	return NULL;

And this one used to return 0 in explicit and returned an empty
string.  The only caller ignored that empty string so returning NULL
in the new code does not make a difference, which also is a good
thing.

So the code looks correctly done.  I just don't get the feeling that
the proposed log message explains the change clearly to readers.

After reading the code through, I think "there's no default case"
was what caused my confusion.

    But unlike remote names, there is no default name when the user
    didn't configure one.  The only way the "explicit" parameter is
    used by the caller is to use the value returned from the helper
    when it is set, and use an empty string otherwise, ignoring the
    returned value from the helper.

    Let's drop the "explicit" out-parameter, and return NULL when
    the returned value from the helper should be ignored, to
    simplify the function interface.

perhaps?

  reply	other threads:[~2020-03-03 18:10 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 [this message]
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=xmqqzhcx8gz8.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 \
    --subject='Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()' \
    /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).