Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:00:50
Hi, 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.
Micronit: nonzero, not true. (true tends to mean '1' while here we have the usual error return of -1. It's kind of annoying that C doesn't have a nice way to distinguish between the usual int return of 0 for success and the usual bool return of true for success.) Looks like a good change. Some tiny nitpicks below. [...]
quoted hunk ↗ jump to hunk
--- a/refs.h +++ b/refs.h@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction); * that the reference should have had before the update, or zeros if * it must not have existed beforehand. */ -void ref_transaction_update(struct ref_transaction *transaction, +int ref_transaction_update(struct ref_transaction *transaction,
The comment above the prototype doesn't tell me: When should the caller expect ref_transaction_update to return an error? What does an error mean: is it always a sign of a bug in the caller, or can it be due to some other problem? What guarantees does the caller have about the state after an error --- is it just "Things will be in a sane state so you can free resources and exit", or will the ref_transaction_update() have been essentially a no-op allowing the caller to continue? [...]
quoted hunk ↗ jump to hunk
--- a/refs.c +++ b/refs.c@@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction, return update; } -void ref_transaction_update(struct ref_transaction *transaction, +int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, int flags, int have_old) { - struct ref_update *update = add_update(transaction, refname); + struct ref_update *update; + + if (have_old && !old_sha1) + return error("have_old is true but old_sha1 is NULL");
I agree with Michael that the error message should start with "BUG:" so humans encountering this know to contact the list instead of blaming themselves. Returning error instead of die-ing seems like a nice thing that make it easier to make git valgrind-clean some day. Others might disagree with me about whether that's worthwhile, but I think it's a good change. :) [...]
quoted hunk ↗ jump to hunk
--- a/builtin/update-ref.c +++ b/builtin/update-ref.c@@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) if (*next != line_termination) die("update %s: extra input: %s", refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old)) + die("failed transaction update for %s", refname);
ref_transaction_update already printed an error, but of course that's
no guarantee that bugs in ref_transaction_update will not cause it
to fail without printing a message in the future. And the extra
context for the error might be nice (but why not print refname in
the message from ref_transaction_update instead?).
Is the plan for ref_transaction_update to be able to fail for
other reasons some day? What is the contract --- do we need a
human-readable, translatable message here, or is a "this can't
happen" BUG message fine?
I'd be fine with
die("BUG: failed transa...
or
/* ref_transaction_update already printed a message */
exit(128)
with a slight preference for the former, for what it's worth.
[...]quoted hunk ↗ jump to hunk
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) if (*next != line_termination) die("verify %s: extra input: %s", refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old)) + die("failed transaction update for %s", refname);
Likewise. Thanks, Jonathan