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. 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? 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". 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.) 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. 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