Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status
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
Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status
On 04/17/2014 09:46 PM, 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.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/update-ref.c | 11 +++
> refs.c | 9 +++--
> refs.h | 10 +-
> 3 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 405267f..12bfacc 100644
> --- 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);
> +
>
> update_flags = 0;
> free(refname);
> @@ -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);
>
> update_flags = 0;
> free(refname);
> diff --git a/refs.c b/refs.c
> index 138ab70..da1761d 100644
> --- 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");
The function documentation doesn't seem to allow old_sha1 == NULL. That
means that calling it that way is a bug, not an error. Therefore, I
think this should result in
die("BUG: 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;
> }
>
> void ref_transaction_create(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index 892c5b6..00e4f7b 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
>
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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
[PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status
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
---
builtin/update-ref.c | 11 +++
refs.c | 9 +++--
refs.h | 10 +-
3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..12bfacc 100644
--- 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);
+
update_flags = 0;
free(refname);
@@ -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);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 138ab70..da1761d 100644
--- 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");
+ 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;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 892c5b6..00e4f7b 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.513.gd486896
--
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
