Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream

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

Jeff King <peff@peff.net> writes:

> On Thu, May 21, 2015 at 11:33:58AM -0700, Junio C Hamano wrote:
>
>> > +static const char *error_buf(struct strbuf *err, const char *fmt, ...)
>> >  {
>> > -	if (!branch || !branch->merge || !branch->merge[0])
>> > -		return NULL;
>> > +	if (err) {
>> > +		va_list ap;
>> > +		va_start(ap, fmt);
>> > +		strbuf_vaddf(err, fmt, ap);
>> > +		va_end(ap);
>> > +	}
>> > +	return NULL;
>> > +}
>> 
>> Many of our functions return -1 to signal an error, and that is why
>> it makes sense for our error() helper to return -1 to save code in
>> the caller, but only because the callers of this private helper use
>> a NULL to signal an error, this also returns NULL.  If we were to
>> use the "callers can opt into detailed message by passing strbuf"
>> pattern more widely, we would want a variant of the above that
>> returns -1, too.  And such a helper would do the same thing as
>> above, with only difference from the above is to return -1.
>
> Yeah, this is not really a good general-purpose purpose function in that
> sense. I have often wanted an error() that returns NULL, but avoided
> adding just because it seemed like needless proliferation.
>
> The real reason to include the return value at all in error() is to let
> you turn two-liners into one-liners.

Yeah, our error() returns -1 to save code in the caller.  And the
callers of this private helper all want to return NULL, so this
returns NULL for the same reason.

> We could drop the return value from
> this helper entirely (or make it -1, but of course no callers would use
> it) and write it out long-hand in the callers:
>
>   if (!branch->merge) {
> 	error_buf(err, ...);
> 	return NULL;
>   }
>
> That is really not so bad, as there are only a handful of callers.

That may happen when somebody (perhaps Jonathan?) wants to push the
"let's optionally pass strbuf to format messages into" approach
forward, and most likely be done by adding a similar function to
usage.c next to error_builtin() and friends.  As long as we do not
forget to reimplement this helper in terms of that public function
when it happens, what we have right now after this patch would be
fine.

> Yeah, my goal here was to faithfully keep the same logic, but I had a
> similar head-scratching moment. What would make more sense to me is:
>
>   if (!branch)
> 	return error("HEAD does not point to a branch");
>
>   if (!branch->merge || !branch->merge[0]) {
> 	/*
> 	 * no merge config; is it because the user didn't define any, or
> 	 * because it is not a real branch, and get_branch auto-vivified
> 	 * it?
> 	 */
> 	if (!ref_exists(branch->refname))
> 		return error("no such branch");
> 	return error("no upstream configured for branch");
>   }
>
>   if (!branch->merge[0]->dst)
> 	return error("upstream branch not stored as a remote-tracking branch");
>
>   return branch->merge[0]->dst;
>
> Hopefully the comment there answers your question; it is not that we
> truly care whether the ref exists, but only that we are trying to tell
> the difference between a "real" branch and a struct that is an artifact
> of our internal code.

Well, answering "my" question here on the list wouldn't help future
readers of the code very much ;-)

> Note also that the original may dereference branch->merge[0] even if it
> is NULL. I think that can't actually happen in practice (we only
> allocate branch->merge if we have at least one item to put in it, and
> all of the checks of branch->merge[0] are actually being over-careful).
> But the code I just wrote above does not have that problem.

Perhaps update the patch with this explanation in the log message,
as a separate preparatory step?

Thanks.