Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

From: Jeff King <peff@peff.net>
Date: Dec 10, 2014, 19:14 UTC
Message-ID: <20141210191434.GA23273@peff.net>
In-Reply-To: <xmqqfvcngwjh.fsf@gitster.dls.corp.google.com>

On Wed, Dec 10, 2014 at 11:00:18AM -0800, Junio C Hamano wrote:

> Two potential issues are:
> 
>  - Callers that ignore errors need to actively ignore errors with
>    strbuf_release(&result.msg);

That was my first thought, too. If you want to do anything besides report_error, you have to deal with the strbuf. But I'd guess that they often fall into one of two cases:

  1. You are just propagating the error to your caller. In which case
      it is not _your_ result struct in the first place, and you do not
      need to care about deallocating it either way. I.e.:

        int some_func(..., struct result *err)
	{
		if (some_other_func(..., err))
			return -1;
		...
	}

  2. You want to ignore the error. I think anybody taking a result
     struct (or a strbuf, or whatever) should accept NULL as "do not
     bother giving me your message". And the convenience wrappers
     should handle that (I think the mkerror example I sent earlier
     did), so callees can just do:

       return mkerror(err, "whatever: %s", ...);

The remainder could strbuf_release manually, but there would hopefully not be many of them.

I think I could live with something like that.

Show 5 quoted lines

>  - Callers have to remember that once the report_errors() function
>    is called on a "struct result", the struct loses its information.
> 
> Neither is insurmountable, but the latter might turn out to be
> cumbersome to work around in some codepaths.

I suspect the message is not that interesting after calling report_errors(). The "code" flag could remain, as it does not require deallocation.

Show 6 quoted lines

> Another alternative may be to have the reporting storage on the side
> of the callee, and have callers that are interested in errors to
> supply a place to store a pointer to it, i.e.
> 
> 	int some_func(..., struct result **errors) {
>         	static struct result mine;

This makes some_func not reentrant. Which is a hazard both for threaded code, but also for functions which want to do:

  if (some_func(foo, &err_one)) {
          /* didn't work? Try an alternative. */
	  if (!some_func(bar, &err_two))
	          ....

and expect err_one to contain anything useful.