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

[Junio C Hamano <gitster@pobox.com>]

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?