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

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

>
>  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.