Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Jeff King p...@peff.net writes: On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote: Whether or not we decide on a different error-handling convention in the future, it is a fact of life that a good bit of code already uses the strbuf convention documented by Jonathan's patch. So I think it is OK to merge it as is. If we change the preferred convention in the future, one part of the change will be to update this file. I wasn't sure about a good bit of code already, but that settles it. Let's take it as-is and see how the code evolves. I'm not convinced myself after a quick grep. But it's certainly non-zero, and I think I would rather have the technique documented than not, so I withdraw my earlier complaints. OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On 02/13/2015 12:08 AM, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote: I am more worried about variable length part pushing the information that is given later out to the right, e.g. error: missing file '%s' prevents us from doing X. Chomping to [1024] is not a good strategy for that kind of message; abbreviating %s to /path/name/... (again, with literally ...) would be. I have this one in my pile of Undecided topics: * jn/doc-api-errors (2014-12-04) 1 commit - doc: document error handling functions and conventions For discussion. What's the status of this one I think we all agree that the early part of the new documentation text is good, but the last section that proposes to store more detailed errors in caller supplied strbuf in textual form was controversial (and I have not convinced myself it is a good idea yet). I could chuck the last section and then start merging the remainder to 'next' to salvage the obviously good bits. Or do people want to hash its last section a bit more? Whether or not we decide on a different error-handling convention in the future, it is a fact of life that a good bit of code already uses the strbuf convention documented by Jonathan's patch. So I think it is OK to merge it as is. If we change the preferred convention in the future, one part of the change will be to update this file. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Michael Haggerty mhag...@alum.mit.edu writes: I think we all agree that the early part of the new documentation text is good, but the last section that proposes to store more detailed errors in caller supplied strbuf in textual form was controversial (and I have not convinced myself it is a good idea yet). I could chuck the last section and then start merging the remainder to 'next' to salvage the obviously good bits. Or do people want to hash its last section a bit more? Whether or not we decide on a different error-handling convention in the future, it is a fact of life that a good bit of code already uses the strbuf convention documented by Jonathan's patch. So I think it is OK to merge it as is. If we change the preferred convention in the future, one part of the change will be to update this file. I wasn't sure about a good bit of code already, but that settles it. Let's take it as-is and see how the code evolves. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote: Whether or not we decide on a different error-handling convention in the future, it is a fact of life that a good bit of code already uses the strbuf convention documented by Jonathan's patch. So I think it is OK to merge it as is. If we change the preferred convention in the future, one part of the change will be to update this file. I wasn't sure about a good bit of code already, but that settles it. Let's take it as-is and see how the code evolves. I'm not convinced myself after a quick grep. But it's certainly non-zero, and I think I would rather have the technique documented than not, so I withdraw my earlier complaints. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote: I am more worried about variable length part pushing the information that is given later out to the right, e.g. error: missing file '%s' prevents us from doing X. Chomping to [1024] is not a good strategy for that kind of message; abbreviating %s to /path/name/... (again, with literally ...) would be. I have this one in my pile of Undecided topics: * jn/doc-api-errors (2014-12-04) 1 commit - doc: document error handling functions and conventions For discussion. What's the status of this one I think we all agree that the early part of the new documentation text is good, but the last section that proposes to store more detailed errors in caller supplied strbuf in textual form was controversial (and I have not convinced myself it is a good idea yet). I could chuck the last section and then start merging the remainder to 'next' to salvage the obviously good bits. Or do people want to hash its last section a bit more? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
On 12/04/2014 08:59 AM, Jeff King wrote:
On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:
The allocation of a variable-sized buffer is a small overhead that I
don't mind incurring on error. In the non-error case, the caller
doesn't actually have to free the buffer, and if they choose to, the
overhead incurred is that of free(NULL)'.
I don't care at all about overhead. I care about extra work on the part
of the caller to avoid a leak. It turns:
if (some_func(fd, err))
return error(%s, err.msg);
into:
if (some_func(fd, err)) {
error(%s, err.buf);
strbuf_release(err);
return -1;
}
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.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Michael Haggerty mhag...@alum.mit.edu writes:
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);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
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.
- 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.
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Jeff King p...@peff.net writes: On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote: I am more worried about variable length part pushing the information that is given later out to the right, e.g. error: missing file '%s' prevents us from doing X. Chomping to [1024] is not a good strategy for that kind of message; abbreviating %s to /path/name/... (again, with literally ...) would be. True. Unfortunately I do not think there is an easy way to truncate the expanded strings used by placeholders... ... Unless we can do something clever with a set of global error strbufs or something (i.e., that expand as needed, but the caller does not have to free themselves, as they will get recycled eventually). That has its own corner cases, though. I do share your concern that strbuf-approach calls for more boilerplate leading to unmaintainable code, but I offhand do not have a magic silver bullet for it. globals are indeed tempting, but I'd have to say that what Jonathan has may probably be the least bad of the possibilities. Chomping also has complications when we have to deal with user payload (e.g. pathname in UTF-8), which we may want avoid having to worry about. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On Tue, Dec 09, 2014 at 10:43:52AM -0800, Junio C Hamano wrote: Unless we can do something clever with a set of global error strbufs or something (i.e., that expand as needed, but the caller does not have to free themselves, as they will get recycled eventually). That has its own corner cases, though. I do share your concern that strbuf-approach calls for more boilerplate leading to unmaintainable code, but I offhand do not have a magic silver bullet for it. globals are indeed tempting, but I'd have to say that what Jonathan has may probably be the least bad of the possibilities. OK. I'm not sure I agree that it is the least bad, but I don't think it's worth arguing over more. Let's go with it, and you can note my objection in the captain's log. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
The only downside I can think of is that we may truncate the message in
exceptional circumstances. But is it really any less helpful to say:
error: unable to open file: some-incredibly-long-filename-aa...
than printing out an extra 100 lines of a? And I mean the ...
literally. I think mkerror() should indicate the truncation with a
..., just so that it is clear to the user. It should almost never
happen, but when it does, it can be helpful to show the user that yes,
we know we are truncating the message, and it is not that git truncated
your filename during the operation.
Is this truncation really a concern, and/or is there some other downside
I'm not thinking of?
I am more worried about variable length part pushing the information
that is given later out to the right, e.g. error: missing file '%s'
prevents us from doing X. Chomping to [1024] is not a good
strategy for that kind of message; abbreviating %s to /path/name/...
(again, with literally ...) would be.
True. Unfortunately I do not think there is an easy way to truncate the
expanded strings used by placeholders. The two approaches I could think
of are:
1. Loop over the va_list and tweak any pointers we find. Except that
loop means we actually need to loop over the _format string_, since
that is the only thing which tells us which placeholders are strings.
But I am not even sure there is a portable way to munge a va_list,
as opposed to just reading it.
2. Transparently rewrite '%s' in the format string to '%.128s' or
similar. This also requires iterating over the format string, but
it's fairly straightforward. Code is below, but it's not _quite_
right. We would want to conditionally add ... based on whether or
not the particular item was truncated. But we cannot know when
munging the format string if that will happen or not (we know
_something_ will be truncated, but not which string).
I don't think you can do it right without reimplementing vsnprintf
yourself. Which is obviously a terrible idea.
I still think truncation is going to be a non-issue in practice, and I'd
rather deal with that potential fallout than have to deal with memory
management in every error handler that uses this technique. But I don't
have much more to say on the matter, so if you disagree, I guess that's
that.
Unless we can do something clever with a set of global error strbufs or
something (i.e., that expand as needed, but the caller does not have to
free themselves, as they will get recycled eventually). That has its own
corner cases, though.
Anyway, the truncating-fmt code is below, for fun.
-Peff
static int abbrev_fmt(char *buf, int len, int abbrev, const char *fmt, va_list
ap)
{
va_list cp;
int got;
va_copy(cp, ap);
got = vsnprintf(buf, len, fmt, ap);
if (got = len) {
struct strbuf munged = STRBUF_INIT;
while (*fmt) {
char ch = *fmt++;
strbuf_addch(munged, ch);
if (ch == '%') {
if (*fmt == 's') {
strbuf_addf(munged, .%ds..., abbrev);
fmt++;
} else /* skips past %%-quoting */
strbuf_addch(munged, *fmt);
}
}
got = vsnprintf(buf, len, munged.buf, cp);
strbuf_release(munged);
}
return got;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Jeff King p...@peff.net writes: The only downside I can think of is that we may truncate the message in exceptional circumstances. But is it really any less helpful to say: error: unable to open file: some-incredibly-long-filename-aa... than printing out an extra 100 lines of a? And I mean the ... literally. I think mkerror() should indicate the truncation with a ..., just so that it is clear to the user. It should almost never happen, but when it does, it can be helpful to show the user that yes, we know we are truncating the message, and it is not that git truncated your filename during the operation. Is this truncation really a concern, and/or is there some other downside I'm not thinking of? I am more worried about variable length part pushing the information that is given later out to the right, e.g. error: missing file '%s' prevents us from doing X. Chomping to [1024] is not a good strategy for that kind of message; abbreviating %s to /path/name/... (again, with literally ...) would be. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Your solution adds a strbuf. That helps with context and stomping, but loses readability and adds allocation. If we changed the strbuf to a fixed-size buffer, that would help the allocation issue. Some messages might be truncated, but it seems unlikely in practice. It still loses readability, though. What about a struct that has an errno-like value _and_ a fixed-size buffer? I'm thinking something like: What do you mean by the allocation being an issue? We're only populating the error buffer in the error case, so you're not talking about performance/speed I'd assume? As error handling breaks in the least expected ways, I'd rather go with well tested string buffer codes there? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote: Your solution adds a strbuf. That helps with context and stomping, but loses readability and adds allocation. If we changed the strbuf to a fixed-size buffer, that would help the allocation issue. Some messages might be truncated, but it seems unlikely in practice. It still loses readability, though. What about a struct that has an errno-like value _and_ a fixed-size buffer? I'm thinking something like: What do you mean by the allocation being an issue? I mean that the caller has to take care of releasing the memory. This adds boilerplate to the caller, and is a potential source of leaks. We're only populating the error buffer in the error case, so you're not talking about performance/speed I'd assume? No, I don't care about performance here. Only code maintainability. As error handling breaks in the least expected ways, I'd rather go with well tested string buffer codes there? We'd still be building on snprintf. And with a known-size buffer, we can wrap the formatting so that the callers don't even have to care (see the mkerror example I posted). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Jonathan Nieder jrnie...@gmail.com writes: Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: -extern int copy_fd(int ifd, int ofd); +extern int copy_fd(int ifd, int ofd, struct strbuf *err); It is not limited to this single function, but what contract do we envision this error messages are given back to the caller via strbuf convention should give between the callers and the callee? Here's a draft for documentation on that. Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition. Documentation/technical/api-error-handling.txt | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/technical/api-error-handling.txt diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt new file mode 100644 index 000..fc68db1 --- /dev/null +++ b/Documentation/technical/api-error-handling.txt @@ -0,0 +1,75 @@ +Error reporting in git +== + +`die`, `usage`, `error`, and `warning` report errors of various +kinds. + +- `die` is for fatal application errors. It prints a message to + the user and exits with status 128. + +- `usage` is for errors in command line usage. After printing its + message, it exits with status 129. (See also `usage_with_options` + in the link:api-parse-options.html[parse-options API].) + +- `error` is for non-fatal library errors. It prints a message + to the user and returns -1 for convenience in signaling the error + to the caller. + +- `warning` is for reporting situations that probably should not + occur but which the user (and Git) can continue to work around + without running into too many problems. Like `error`, it + returns -1 after reporting the situation to the caller. + +Customizable error handlers +--- + +The default behavior of `die` and `error` is to write a message to +stderr and then exit or return as appropriate. This behavior can be +overridden using `set_die_routine` and `set_error_routine`. For +example, git daemon uses set_die_routine to write the reason `die` +was called to syslog before exiting. + +Library errors +-- + +Functions return a negative integer on error. Details beyond that +vary from function to function: + +- Some functions return -1 for all errors. Others return a more + specific value depending on how the caller might want to react + to the error. + +- Some functions report the error to stderr with `error`, + while others leave that for the caller to do. + +- errno is not meaningful on return from most functions (except + for thin wrappers for system calls). + +Check the function's API documentation to be sure. + +Caller-handled errors +- + +An increasing number of functions take a parameter 'struct strbuf *err'. +On error, such functions append a message about what went wrong to the +'err' strbuf. The message is meant to be complete enough to be passed +to `die` or `error` as-is. For example: + + if (ref_transaction_commit(transaction, err)) + die(%s, err.buf); + +The 'err' parameter will be untouched if no error occured, so multiple +function calls can be chained: + + t = ref_transaction_begin(err); + if (!t || + ref_transaction_update(t, HEAD, ..., err) || + ret_transaction_commit(t, err)) + die(%s, err.buf); + +The 'err' parameter must be a pointer to a valid strbuf. To silence +a message, pass a strbuf that is explicitly ignored: + + if (thing_that_can_fail_in_an_ignorable_way(..., err)) + /* This failure is okay. */ + strbuf_reset(err); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Hi, Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Here's a draft for documentation on that. Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition. Thanks. Can you say more? I didn't think the discussion had reached a conclusion yet. Unless there's a strong reason to do otherwise, I prefer to stick with the strbufs in this series for now. I prefer strbuf for this purpose over char[1024]. And switching later doesn't seem hard. But if others prefer char[1024], I can update the existing code that uses strbuf from the transaction API to use that and reroll this series with that convention. A part of me dislikes that but I'll go along. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Here's a draft for documentation on that. Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition. I think I see what I misunderstood. Do you mean even if we settle on a different API, this documentation of what you started with should be easy to adapt and will make life easier? In other words, did you mean to say that things are still up in the air (which I agree with) or that the project has already settled on a different direction (which I do not)? Sorry for the confusion, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote: Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Here's a draft for documentation on that. Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition. I think I see what I misunderstood. Do you mean even if we settle on a different API, this documentation of what you started with should be easy to adapt and will make life easier? In other words, did you mean to say that things are still up in the air (which I agree with) or that the project has already settled on a different direction (which I do not)? FWIW, that is how I read it (up in the air), and where I thought our discussion was. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Yeah, that is what I meant. The earlier part will not go to waste no matter what happens to the discussion. I am not a fan of char[1024], if only because our error message may have to mention things whose length is not under our control, e.g. a filename in the working tree, but I do share your concern that strbuf-approach calls for more boilerplate. I offhand do not have a magic silver bullet for it, though. On Thu, Dec 4, 2014 at 3:44 PM, Jeff King p...@peff.net wrote: On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote: Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Here's a draft for documentation on that. Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition. I think I see what I misunderstood. Do you mean even if we settle on a different API, this documentation of what you started with should be easy to adapt and will make life easier? In other words, did you mean to say that things are still up in the air (which I agree with) or that the project has already settled on a different direction (which I do not)? FWIW, that is how I read it (up in the air), and where I thought our discussion was. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
On Thu, Dec 04, 2014 at 03:52:45PM -0800, Junio C Hamano wrote: Yeah, that is what I meant. The earlier part will not go to waste no matter what happens to the discussion. I am not a fan of char[1024], if only because our error message may have to mention things whose length is not under our control, e.g. a filename in the working tree, but I do share your concern that strbuf-approach calls for more boilerplate. I offhand do not have a magic silver bullet for it, though. The only downside I can think of is that we may truncate the message in exceptional circumstances. But is it really any less helpful to say: error: unable to open file: some-incredibly-long-filename-aa... than printing out an extra 100 lines of a? And I mean the ... literally. I think mkerror() should indicate the truncation with a ..., just so that it is clear to the user. It should almost never happen, but when it does, it can be helpful to show the user that yes, we know we are truncating the message, and it is not that git truncated your filename during the operation. Is this truncation really a concern, and/or is there some other downside I'm not thinking of? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Jonathan Nieder jrnie...@gmail.com writes: -extern int copy_fd(int ifd, int ofd); +extern int copy_fd(int ifd, int ofd, struct strbuf *err); It is not limited to this single function, but what contract do we envision this error messages are given back to the caller via strbuf convention should give between the callers and the callee? For example, is it a bug in the callee to touch err when there is no error to report? Another example is if we should allow the callers to pass NULL there when they do not care about the nature of the error (e.g. git cmd -q). There may be other rules we want to enforce consistently across functions that adopt this convention. The change in this patch looks sensible. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Jonathan Nieder jrnie...@gmail.com writes: This way, callers can put the message in context or even avoid printing the message altogether. Currently hold_lock_file_for_append tries to save errno in order to produce a meaningful message about the failure and tries to print a second message using errno. Unfortunately the errno it uses is not meaningful because copy_fd makes no effort to set a meaningful errno value. This change is preparation for simplifying the hold_lock_file_for_append behavior. No user-visible change intended yet. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- The title feature. By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Junio C Hamano wrote: By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead? Yes, please. I'll give it an hour or two to collect more comments and then send a reroll reflecting them. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
On Wed, Dec 3, 2014 at 12:02 PM, Junio C Hamano gits...@pobox.com wrote: Jonathan Nieder jrnie...@gmail.com writes: This way, callers can put the message in context or even avoid printing the message altogether. Currently hold_lock_file_for_append tries to save errno in order to produce a meaningful message about the failure and tries to print a second message using errno. Unfortunately the errno it uses is not meaningful because copy_fd makes no effort to set a meaningful errno value. This change is preparation for simplifying the hold_lock_file_for_append behavior. No user-visible change intended yet. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- The title feature. By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead? This series makes the code more readable and maintainable in the end as we don't need to hunt down the path of when the errno is changed or accessed. The currently cooking sb/copy-fd-errno is more of a hot fix, while this series is fixing the problem more at the basic level and more in long term. I'd be happy if this replaces the currently cooking version. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: -extern int copy_fd(int ifd, int ofd); +extern int copy_fd(int ifd, int ofd, struct strbuf *err); It is not limited to this single function, but what contract do we envision this error messages are given back to the caller via strbuf convention should give between the callers and the callee? Good point. I'll add a Documentation/technical/api-error-handling.txt describing die(), error(), warning(), and the strbuf-based method. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
On Wed, Dec 03, 2014 at 11:01:08AM -0800, Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
It is not limited to this single function, but what contract do we
envision this error messages are given back to the caller via
strbuf convention should give between the callers and the callee?
For example, is it a bug in the callee to touch err when there is
no error to report? Another example is if we should allow the
callers to pass NULL there when they do not care about the nature of
the error (e.g. git cmd -q).
There may be other rules we want to enforce consistently across
functions that adopt this convention.
It seems like we are really re-designing the error-handling chain here.
Maybe it is worth taking a step back and thinking about our overall
strategy.
Why do we dislike errno? Because it is global state, and it is easy for
it to get stomped by unrelated operations. Another reason is that it
carries no parameters. You see ENOENT, but you do not have any context
(e.g., which file).
What's good about errno? It is machine-readable (i.e., callers can check
for ENOENT, as opposed to text in a strbuf). It does not require any
allocation. Besides making it slightly more robust, it removes any
responsibility for resource cleanup from the caller. It's globalness is
also convenient; you do not need to add an extra parameter to each
function to handle errors.
So what are some alternatives?
Passing back -errno instead of -1 helps with the stomping issue (and
without adding extra parameters). It also retains machine-readability
(which I'll call just readability from now on). But it doesn't help
with context, or better messaging.
Your solution adds a strbuf. That helps with context and stomping, but
loses readability and adds allocation.
If we changed the strbuf to a fixed-size buffer, that would help the
allocation issue. Some messages might be truncated, but it seems
unlikely in practice. It still loses readability, though.
What about a struct that has an errno-like value _and_ a fixed-size
buffer? I'm thinking something like:
struct error {
int code;
char msg[1024];
};
/* caller who wants to print the message; no need to free message */
struct error err;
if (copy_fd(from, to, err))
return error(%s, err.msg);
/* caller who does not; they can pass NULL */
if (copy_fd(from, to, NULL))
return -1;
/* or they can use it to grab the errno value */
struct error err;
if (copy_fd(from, to, err)) {
if (err.code == EPIPE)
exit(141);
return -1;
}
/* function which generates error */
void read_foo(const char *fn, struct error *err)
{
if (open(fn, O_RDONLY))
return mkerror(err, errno, unable to open %s, fn);
... do other stuff ...
return 0;
}
/* helper function for generating errors */
int mkerror(struct error *err, int code, const char *fmt, ...)
{
va_list ap;
int len;
if (!err)
return;
err-code = code;
va_start(ap, fmt);
len = vsnprintf(err-msg, sizeof(err-msg), fmt, ap);
va_end(ap);
if (code)
snprintf(err-msg + len, sizeof(err-msg) - len,
: %s, strerror(code));
return -1;
}
You can also take the machine-readable thing a step further, like:
struct error {
int code;
char param1[1024];
char param2[1024];
/* 2 parameters should be big enough for anyone, right? */
}
and then generate the message on the fly when printing. This gives the
callers more information. But it also means defining a constant for
code for every error message, which is annoying. Libraries often do
this, but I do not think we need to go that far here.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Hi,
Jeff King wrote:
What about a struct that has an errno-like value _and_ a fixed-size
buffer? I'm thinking something like:
struct error {
int code;
char msg[1024];
};
My experience with errno is that it is very hard to anticipate what
granularity to use with error codes, especially if the error code is
going to (in some contexts) determine the message printed to the user.
In practice, it is easier to come up with a message at the error
detection site and use a generic something happened error code
except in those places where the caller is going to act on specific
error types that need to be distinguished.
Passing a message back through a strbuf leaves room for such
interpretations for the int return value.
The 0 means error convention gives room to use different exit
codes for the errors that need to be programmatically distinguished.
For example, the ref transaction API uses this to distinguish D/F
conflicts from other errors, while still returning an error message
through a strbuf output parameter.
This doesn't help with functions that want to return a pointer (and
NULL on error). For those, we can use an int output parameter, copy
the kernel's err_ptr trick, or use other sentinels for recoverable
errors that the caller wants to know about. It hasn't come up in
practice enough for me to be motivated to want to choose between
those.
If we changed the strbuf to a fixed-size buffer, that would help the
allocation issue. Some messages might be truncated, but it seems
unlikely in practice. It still loses readability, though.
I don't like the idea of having to choose a size in advance and not
being able to fit more detailed (perhaps language-specific, and
including user-specified input) messages lest the buffer overflow.
The allocation of a variable-sized buffer is a small overhead that I
don't mind incurring on error. In the non-error case, the caller
doesn't actually have to free the buffer, and if they choose to, the
overhead incurred is that of free(NULL)'.
My two cents,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead? Yes, please. OK. It felt strange that two people in a same office stomping on each other's toes, which I interpreted as an apparent lack of communication, but perhaps there was an off-line communication between you two with an understanding that this will supersede the other one, which I probably failed to spot in the cover letter. I'll give it an hour or two to collect more comments and then send a reroll reflecting them. Make that one revolution of earth, perhaps? e.g. http://thread.gmane.org/gmane.comp.version-control.git/259831/focus=260349 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Junio C Hamano wrote: Jonathan Nieder writes: Junio C Hamano wrote: By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead? Yes, please. OK. It felt strange that two people in a same office stomping on each other's toes I just did a bad job of explaining the context in the cover letter. I wrote this series at Stefan's request (see the message that the cover letter is a response to for context[1]). Hope that helps, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259710 and http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259703 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)
Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: -extern int copy_fd(int ifd, int ofd); +extern int copy_fd(int ifd, int ofd, struct strbuf *err); It is not limited to this single function, but what contract do we envision this error messages are given back to the caller via strbuf convention should give between the callers and the callee? Here's a draft for documentation on that. Documentation/technical/api-error-handling.txt | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/technical/api-error-handling.txt diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt new file mode 100644 index 000..fc68db1 --- /dev/null +++ b/Documentation/technical/api-error-handling.txt @@ -0,0 +1,75 @@ +Error reporting in git +== + +`die`, `usage`, `error`, and `warning` report errors of various +kinds. + +- `die` is for fatal application errors. It prints a message to + the user and exits with status 128. + +- `usage` is for errors in command line usage. After printing its + message, it exits with status 129. (See also `usage_with_options` + in the link:api-parse-options.html[parse-options API].) + +- `error` is for non-fatal library errors. It prints a message + to the user and returns -1 for convenience in signaling the error + to the caller. + +- `warning` is for reporting situations that probably should not + occur but which the user (and Git) can continue to work around + without running into too many problems. Like `error`, it + returns -1 after reporting the situation to the caller. + +Customizable error handlers +--- + +The default behavior of `die` and `error` is to write a message to +stderr and then exit or return as appropriate. This behavior can be +overridden using `set_die_routine` and `set_error_routine`. For +example, git daemon uses set_die_routine to write the reason `die` +was called to syslog before exiting. + +Library errors +-- + +Functions return a negative integer on error. Details beyond that +vary from function to function: + +- Some functions return -1 for all errors. Others return a more + specific value depending on how the caller might want to react + to the error. + +- Some functions report the error to stderr with `error`, + while others leave that for the caller to do. + +- errno is not meaningful on return from most functions (except + for thin wrappers for system calls). + +Check the function's API documentation to be sure. + +Caller-handled errors +- + +An increasing number of functions take a parameter 'struct strbuf *err'. +On error, such functions append a message about what went wrong to the +'err' strbuf. The message is meant to be complete enough to be passed +to `die` or `error` as-is. For example: + + if (ref_transaction_commit(transaction, err)) + die(%s, err.buf); + +The 'err' parameter will be untouched if no error occured, so multiple +function calls can be chained: + + t = ref_transaction_begin(err); + if (!t || + ref_transaction_update(t, HEAD, ..., err) || + ret_transaction_commit(t, err)) + die(%s, err.buf); + +The 'err' parameter must be a pointer to a valid strbuf. To silence +a message, pass a strbuf that is explicitly ignored: + + if (thing_that_can_fail_in_an_ignorable_way(..., err)) + /* This failure is okay. */ + strbuf_reset(err); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:
What about a struct that has an errno-like value _and_ a fixed-size
buffer? I'm thinking something like:
struct error {
int code;
char msg[1024];
};
My experience with errno is that it is very hard to anticipate what
granularity to use with error codes, especially if the error code is
going to (in some contexts) determine the message printed to the user.
In practice, it is easier to come up with a message at the error
detection site and use a generic something happened error code
except in those places where the caller is going to act on specific
error types that need to be distinguished.
Yeah, I agree. I think error.msg there would be the main use, and
error.code is just gravy. I agree it could simply come back through the
integer return value, too.
The 0 means error convention gives room to use different exit
codes for the errors that need to be programmatically distinguished.
For example, the ref transaction API uses this to distinguish D/F
conflicts from other errors, while still returning an error message
through a strbuf output parameter.
Yup. One nice thing about stuffing it into the error struct is that it
saves the caller from declaring a separate variable (assuming they need
err anyway to collect the message). I.e.:
struct error err;
if (some_func(fd, err))
react_to(err.code);
as opposed to:
struct error err;
int ret;
ret = some_func(fd, err);
if (ret)
react_to(ret);
But I think it's a minority of cases where we care about the specific
value anyway, so it's probably not a big deal either way.
If we changed the strbuf to a fixed-size buffer, that would help the
allocation issue. Some messages might be truncated, but it seems
unlikely in practice. It still loses readability, though.
I don't like the idea of having to choose a size in advance and not
being able to fit more detailed (perhaps language-specific, and
including user-specified input) messages lest the buffer overflow.
Is that really a problem in practice? Is a message larger than 1024
characters actually readable? It would have to be broken across many
lines, and then I think we are no longer dealing with an error message,
but some advice (which probably shouldn't go through this mechanism
anyway).
The allocation of a variable-sized buffer is a small overhead that I
don't mind incurring on error. In the non-error case, the caller
doesn't actually have to free the buffer, and if they choose to, the
overhead incurred is that of free(NULL)'.
I don't care at all about overhead. I care about extra work on the part
of the caller to avoid a leak. It turns:
if (some_func(fd, err))
return error(%s, err.msg);
into:
if (some_func(fd, err)) {
error(%s, err.buf);
strbuf_release(err);
return -1;
}
It may make things a little easier when an intermediate function has
to further munge the message. For example, your 04/14 uses
strbuf_prefixf. But that can also be expressed in a buffer world as:
/* parent_err is passed to us from caller, err is a local buffer */
if (copy_fd(orig, fd, err))
return mkerror(parent_err, cannot copy '%s': %s, path, err.msg);
Strbufs make more complicated munging possible, but I'd expect prefixing
to be the most common case (except for not munging at all, which I think
is even more common).
I dunno. I just hate to see a system where it's very easy for callers to
introduce memory leaks by forgetting a strbuf_release, or that bogs
callers down with error-handling boilerplate.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/14] copy_fd: pass error message back through a strbuf
This way, callers can put the message in context or even avoid
printing the message altogether.
Currently hold_lock_file_for_append tries to save errno in order to
produce a meaningful message about the failure and tries to print a
second message using errno. Unfortunately the errno it uses is not
meaningful because copy_fd makes no effort to set a meaningful errno
value. This change is preparation for simplifying the
hold_lock_file_for_append behavior.
No user-visible change intended yet.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
The title feature.
cache.h| 2 +-
convert.c | 6 +-
copy.c | 32 +---
lockfile.c | 6 +-
4 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index 99ed096..ddaa30f 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git a/convert.c b/convert.c
index 9a5612e..e301447 100644
--- a/convert.c
+++ b/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git a/copy.c b/copy.c
index f2970ec..828661a 100644
--- a/copy.c
+++ b/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ strbuf_addf(err, read returned %s, strerror(errno));
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ strbuf_addf(err, write returned %s, strerror(errno));
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
int copy_file(const char *dst, const char *src, int mode)
{
- int fdi, fdo, status;
+ int fdi, fdo;
+ struct strbuf err = STRBUF_INIT;
mode = (mode 0111) ? 0777 : 0666;
if ((fdi = open(src, O_RDONLY)) 0)
@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
close(fdi);
return fdo;
}
- status = copy_fd(fdi, fdo);
+ if (copy_fd(fdi, fdo, err)) {
+ close(fdi);
+ close(fdo);
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ strbuf_release(err);
close(fdi);
if (close(fdo) != 0)
return error(%s: close error: %s, dst, strerror(errno));
-
- if (!status adjust_shared_perm(dst))
+ if (adjust_shared_perm(dst))
return -1;
- return status;
+ return 0;
}
int copy_file_with_time(const char *dst, const char *src, int mode)
diff --git a/lockfile.c b/lockfile.c
index 4f16ee7..b3020f3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -182,6 +182,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
int hold_lock_file_for_append(struct lock_file *lk, const char *path, int
flags)
{
int fd, orig_fd;
+ struct strbuf err = STRBUF_INIT;
fd = lock_file(lk, path, flags);
if (fd 0) {
@@ -202,9 +203,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
errno = save_errno;
return -1;
}
- } else if (copy_fd(orig_fd, fd)) {
+ } else if (copy_fd(orig_fd, fd, err)) {
int save_errno = errno;
+ error(copy-fd: %s, err.buf);
