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 <sahlb...@google.com> > --- > 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? 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. * 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). * 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. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
