Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 3, 2020, 17:51 UTC
- Message-ID
- <xmqqzhcx8gz8.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20200303161223.1870298-2-damien.olivier.robert+git@gmail.com>
Damien Robert <damien.olivier.robert@gmail.com> writes:
Show 10 quoted lines
> 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.
Show 9 quoted lines
> } 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.
Show 18 quoted lines
> 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.
Show 9 quoted lines
> }
> } 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.
Show 7 quoted lines
> } > } > } > - 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?