On Wed, May 14, 2014 at 4:40 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Ronnie Sahlberg wrote: > >> Update ref_transaction_update() do some basic error checking and return >> true on error. Update all callers to check ref_transaction_update() for error. >> There are currently no conditions in _update that will return error but there >> will be in the future. >> >> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> >> --- >> builtin/update-ref.c | 10 ++++++---- >> refs.c | 9 +++++++-- >> refs.h | 10 +++++----- >> 3 files changed, 18 insertions(+), 11 deletions(-) > > Revisiting comments from [1]: > > * When I call ref_transaction_update, what does it mean that I get > a nonzero return value? Does it mean the _update failed and had > no effect? What will I want to do next: should I try again or > print an error and exit? It means the transaction will no longer work and must be rolled back. See below for the updated text I added to refs.h > > Ideally I should be able to answer these questions by reading > the signature of ref_transaction_update and the comment documenting > it. The comment doesn't say anything about what errors > mean here. I have updated the description to include : * Function returns 0 on success and non-zero on failure. A failure to update * means that the transaction as a whole has failed and will need to be * rolled back. > > * the error message change for the have_old && !old_sha1 case (to add > "BUG:" so users know the impossible has happened and translators > know not to bother with it) seems to have snuck ahead into patch 28 > (refs.c: add transaction.status and track OPEN/CLOSED/ERROR). Done. > > * It would be easier to make sense of the error path (does the error > message have enough information? Will the user be bewildered?) > if there were an example of how ref_transaction_update can fail. > > There still doesn't seem to be one by the end of the series. This patch series got a lot longer than I initially thought so I did not get to the point where we it would make sense to start returning !0. :-( The next patchseries I sent out for review does add things to _update that will cause it to return failures. For example, locking the ref there happens in _update instead of _commit and then it starts make sense to return failures back to the caller for things such as "Multiple updates for ref '%s' not allowed." Unfortunate but since this patch series reached >40 patches I did not want to continue expanding on it. This means that actually starting to use "let _update return error" did not actually start becomming used until the second patch series, which now is well over 30 patches in size :-( I just felt I had to stop growing this series or it would never be finished. > > The general idea still seems sensible. > > Thanks, > Jonathan > > [1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115 -- 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