Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> Do any callers care about errno? Does the function's API
> documentation say it will make errno meaningful on error, so people
> making changes to copy_fd in the future know to maintain that
> property?
>
> *searches*
>
> Looks like callers are:
>
> convert.c::filter_buffer_or_fd, which doesn't care
>
> copy.c::copy_file, which also doesn't care
>
> lockfile.c::hold_lock_file_for_append, which started caring
> in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
> before returning, 2014-10-01). But no callers of that function
> care yet.
>
> So this is about fixing a bug-waiting-to-happen in
> hold_lock_file_for_append. That would be enough to motivate the
> change.
OK. Perhaps convert.c wants to be fixed?
>> + int save_errno = errno;
>> + error("copy-fd: read returned %s", strerror(errno));
>> + errno = save_errno;
>> + return -1;
>
> Any caller is presumably going to turn around and print strerror(errno)
> again, producing repetitive output.
>
> Can we do better? E.g., if the signature were
>
> int copy_fd(int ifd, int ofd, struct strbuf *err);
>
> then we could write the error message to the err strbuf for the
> caller to print.
The problem you are solving is "because the caller may want to do
its own error message, stop the callee from emitting the error
message unconditionally", but if we are addressing "the caller may
want to...", I think we should find a single solution that addresses
other kind fo things the caller may want to do.
For example, two callers may want to phrase the same error condition
differently, e.g. depending on what the user asked to do. We'd want
something better than the ERRORMSG trick used in unpack-trees.c,
which does not scale, and I think passing some data that represents
"here is how the caller wants the errors to be handled and
presented" is probably a part of the solution, but strbuf *err is
not that.
In short I am not a very big fan of passing around strbuf *err to
low level helper API functions myself.
But the approach does not make things much worse than it currently
is, other than code churns to pass an extra pointer around.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html