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


To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to