diff mbox series

[v2,1/2] remote: drop "explicit" parameter from remote_ref_for_branch()

Message ID 20200303161223.1870298-2-damien.olivier.robert+git@gmail.com (mailing list archive)
State New, archived
Headers show
Series | expand

Commit Message

Damien Robert March 3, 2020, 4:12 p.m. UTC 
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. In any case where we don't set "explicit", we'd just an empty
string anyway. Let's instead return NULL in this case, letting us
simplify the function interface.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 ref-filter.c |  6 ++----
 remote.c     | 11 ++---------
 remote.h     |  3 +--
 3 files changed, 5 insertions(+), 15 deletions(-)

Comments

Junio C Hamano March 3, 2020, 5:51 p.m. UTC | #1 
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?
Jeff King March 3, 2020, 9:11 p.m. UTC | #2 
On Tue, Mar 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:

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

Maybe more clear:

For remote names, we will always return one of two things:

  - a remote name based on user config, in which case explicit=1

  - the default string "origin", in which case explicit=0

For remote branches, we will return either:

  - the remote branch name from config, in which case explicit=1

  - the empty string, in which case explicit=0

But nobody ever looks at that empty string. For the second case, we
could just as well return NULL. At which point we don't need an explicit
flag at all, as the caller can just check for NULL.

(written before reading what you wrote below)

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

Yes, that looks fine to me.

-Peff
Junio C Hamano March 3, 2020, 10:22 p.m. UTC | #3 
Jeff King <peff@peff.net> writes:

> On Tue, Mar 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:
>
>> > 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.
>
> Maybe more clear:
> ...
> Yes, that looks fine to me.

Thanks for a clarification.
diff mbox series

Patch

diff --git a/ref-filter.c b/ref-filter.c
index 6867e33648..9837700732 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1459,12 +1459,10 @@  static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			remote_for_branch(branch, &explicit);
 		*s = xstrdup(explicit ? remote : "");
 	} 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 : "");
 	} else
 		BUG("unhandled RR_* enum");
 }
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];
 			}
 		} 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;
 			}
 		}
 	}
-	if (explicit)
-		*explicit = 0;
-	return "";
+	return NULL;
 }
 
 static struct remote *remote_get_1(const char *name,
diff --git a/remote.h b/remote.h
index b134cc21be..11d8719b58 100644
--- a/remote.h
+++ b/remote.h
@@ -261,8 +261,7 @@  struct branch {
 struct branch *branch_get(const char *name);
 const char *remote_for_branch(struct branch *branch, int *explicit);
 const char *pushremote_for_branch(struct branch *branch, int *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);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);