On Thu, May 22, 2014 at 4:08 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Ronnie Sahlberg wrote: > >> This patch series can also be found at >> https://github.com/rsahlberg/git/tree/ref-transactions > > Continuing with the review of 65a1cb7b (2014-05-22 12:08): > > 11/40 change ref_transaction_update() to do error checking and return status > The "there will be in the future" sounds ominous. Do you have an > example in mind? E.g., I suppose it would be nice if _update could > notice D/F conflicts or connection to a database server closing early, > but it's not clear to me whether the kind of errors you're talking > about are that or something else. > Updated the message. Next series moves both locking as well as checking for name conflicts to _update. > With or without such a clarification, > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 12/40 change ref_transaction_create to do error checking and return status > What does "On failure the err buffer will be updated" mean? Will > it clear err and replace it with a message, append to err, or > something else? Does the message explain the context or is the > caller responsible for adding to it? Does the message end with a > newline or is the caller responsible for adding one when printing it > out? I have updated the documentation. Message is appended to the string buffer. Caller is required to strbuf_reset before calling the transaction if caller wants only most recent error instead of all errors appended one by one. > > For cases like this where lots of functions have a similar API, > API comments start to become potentially repetitive. It might be > better to explain conventions at the top of the file or in > Documentation/technical/api-refs.txt and say "See the top of the > file for error handling conventions" or "Returns non-zero and > appends a message to err on error. See > Documentation/technical/api-refs.txt for more details on error > handling". Done. > > 13/40 ref_transaction_delete to check for error and return status > Each successive commit dropped something from its subject. :) > (First the (), then the verb.) Done. > > Same comments as before about an example being useful for the > log message and the API documentation on error handling being a > bit vague. > > 14/40 make ref_transaction_begin take an err argument > I found the "failed to connect to mysql" example instructive while > doing reviews. Perhaps it would be worth mentioning in the commit > message. > > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 15/40 add transaction.status and track OPEN/CLOSED/ERROR > It says an ERRORed transaction cannot be committed and can be rolled > back by calling _free. Can a CLOSED transaction be committed or > _freed? > > What does "faild" mean in the documentation comments? (Maybe > "non-OPEN"?) > > In the previous version of this patch passing a non-OPEN transaction > would die("BUG: "...) to diagnose the caller's mistake. Now I'm > confused about the API: it seems you're allowed to pass a non-OPEN > transaction but it doesn't append a message to 'err' in that case. > Is this meant as a way to save the caller some typing, like > fwrite/fclose do? (I've found people often make mistakes with the > fwrite API fwiw but can understand the appeal of it.) > > Maybe with more context I'd like this. As is, it feels like a step > in the wrong direction. > > 16/40 tag: use ref transactions when doing updates > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 17/40 replace: use ref transactions when doing updates > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 18/40 commit: use ref transactions for updates > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 19/40 sequencer: use ref transactions for all ref updates > This would be a lot simpler if the "ref_transaction_commit should not > free the transaction" patch came before it (yes, sorry, killing the > fun). I can push the result of a rebase doing that somewhere if you > like. Beeing done. > > 20/40 fast-import: change update_branch to use ref transactions > Likewise. -- 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