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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 10, 2014, 19:00 UTC
Message-ID: <xmqqfvcngwjh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <54887CB7.4000603@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 45 quoted lines

> What if we go in the direction not of less infrastructure, but a little
> bit more? Like
>
> 	struct result {
> 		int code;
> 		struct strbuf msg;
> 	};
>
> 	int report_errors(struct result *result)
> 	{
> 		int code = result->code;
> 		if (code) {
> 			error(result->msg.buf);
> 		}
> 		result->code = 0;
> 		strbuf_release(result->msg);
> 		return code; /* or alternatively (code ? -1 : 0) */
> 	}
>
> 	int report_warnings(struct result *result)
> 	{
> 		...
> 	}
>
> 	int report_with_prefix(struct result *result, const char *fmt, ...)
> 	{
> 		...
> 	}
>
> Then a caller could look pretty much like before:
>
> 	struct result result = RESULT_INIT;
>
> 	if (some_func(fd, &result))
> 		return report_errors(&result);
>
> Other callers might not even bother to check the return value of the
> function, relying instead on result.code via process_error():
>
> 	char *ptr = some_func(fd, &result);
> 	if (report_errors(&result))
> 		return -1;
>
> If the result code is considered superfluous, we could use naked strbufs
> and use msg.len as the indicator that there was an error.

Two potential issues are:

 - Callers that ignore errors need to actively ignore errors with
   strbuf_release(&result.msg);

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

Other than that, I think it is OK.

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;

		clear_result(&mine);
		... do its thing ...
                if (... error detected ...) {
                        mine.code = E_SOMEFUNC;
                        strbuf_addstr(&mine.msg, "some_func: foobared");
		}

		if (errors)
			*errors = &mine;
                return mine.code;
	}

And a caller would do this instead:

	struct result *result = NULL;

        if (some_func(fd, &result))
		return report_errors(result);

where report_errors() and friends will only peek but not clear the result reporting storage. The clear_result() helper would trivially be:

	void clear_result(struct result *result) {
		result->code = 0;
                strbuf_release(&result->msg);
	}