Re: [PATCH v8 00/44] Use ref transactions for all ref updates

[Jonathan Nieder <jrnieder@gmail.com>]

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@gmail.com>

 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@gmail.com>

 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@gmail.com>

 17/40 replace: use ref transactions when doing updates
 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

 18/40 commit: use ref transactions for updates
 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
 
 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.