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.
[...]
> --- 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?
[...]
> --- 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. :)
[...]
> --- 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.
[...]
> @@ -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
--
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
