Re: [PATCH v8 00/44] Use ref transactions for all ref updates
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:01:18
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 [off-list ref]
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 [off-list ref]
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 [off-list ref]
17/40 replace: use ref transactions when doing updates
Reviewed-by: Jonathan Nieder [off-list ref]
18/40 commit: use ref transactions for updates
Reviewed-by: Jonathan Nieder [off-list ref]
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.