On Mon, Apr 28, 2014 at 5:51 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg <sahlberg@xxxxxxxxxx> 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.
>>
>> 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(-)
>>
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 2bef2a0..59c4d6b 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -197,8 +197,9 @@ 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("update %s: failed", refname);
>>
>> update_flags = 0;
>> free(refname);
>> @@ -286,8 +287,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);
>>
>> update_flags = 0;
>> free(refname);
>> diff --git a/refs.c b/refs.c
>> index 308e13e..1a903fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3333,19 +3333,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)
>> + die("have_old is true but old_sha1 is NULL");
>>
>> + update = add_update(transaction, refname);
>> hashcpy(update->new_sha1, new_sha1);
>> update->flags = flags;
>> update->have_old = have_old;
>> if (have_old)
>> hashcpy(update->old_sha1, old_sha1);
>> + return 0;
>
> Am I misreading? The commit message talks about returning true on
> error, but this code seems to only ever die() or return 0 (false).
>
I have updated the commit message to be more precise.
The function returns true on error but there are currently no checks
that actually
do return failure in this function. Such checks will be added in the future.
Also having it return an int instead of void allows us to use the pattern
if(!transaction ||
ref_transaction_update(transaction, ...) |||
ref_transaction_commit(transaction, ...)) {
eventhough we do not yet have any conditions where _update will fail.
>> }
>>
>> void ref_transaction_create(struct ref_transaction *transaction,
>> diff --git a/refs.h b/refs.h
>> index bc7715e..0364a3e 100644
>> --- 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,
>> - const char *refname,
>> - const unsigned char *new_sha1,
>> - const unsigned char *old_sha1,
>> - int flags, int have_old);
>> +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);
>>
>> /*
>> * Add a reference creation to transaction. new_sha1 is the value
>> --
>> 1.9.1.528.g98b8868.dirty
--
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