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 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)
Michael Haggerty 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)
On 02/13/2015 12:08 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Jeff King 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)
Junio C Hamano writes: > Jeff King 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/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)
Jeff King 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 Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote: > Jeff King 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 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/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/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 wrote: > On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote: > >> Junio C Hamano wrote: >> > Jonathan Nieder 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:41:47PM -0800, Jonathan Nieder wrote: > Junio C Hamano wrote: > > Jonathan Nieder 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)
Junio C Hamano wrote: > Jonathan Nieder 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)
Hi, Junio C Hamano wrote: > Jonathan Nieder 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)
Jonathan Nieder writes: > Signed-off-by: Jonathan Nieder > --- > Junio C Hamano wrote: >> Jonathan Nieder 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
[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 --- Junio C Hamano wrote: > Jonathan Nieder 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