Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string
Good points.
On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as
update_ref_lock
returns, modulo the initial error:/fatal: preamble.
s/returns/prints/?
Done, and then was deleted when I reworded the message.
This will make it easier for callers to craft better error messages when
a transaction call fails.
Interesting. Can you give an example? What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?
I was thinking a bit too far ahead. You could in theory keep logging multiple
lock failures during the _commit() and then when the transaction fails
and returns
it will have appended a list of all refs that failed and not just the
first ref that failed.
I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
-const char *msg, enum action_on_err onerr);
+const char *msg, char **err,
+enum action_on_err onerr);
Is the idea that if I pass in a pointer err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?
Temporarily, yes.
Shortly after this patch I remove the onerr argument completely.
But I want to keep the pass error back to caller and get rid of
onerr as two separate patches.
I think it is easier to follow the flow of changes if they are done in
two separate steps.
Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).
See above.
[...]
--- a/refs.c
+++ b/refs.c
[...]
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err) {
+ const char *str = Cannot lock the ref '%s'.;
+ *err = xmalloc(PATH_MAX + 24);
+ snprintf(*err, PATH_MAX + 24, str,
+ update-refname);
+ }
Might be clearer to use a helper similar to path.c::mkpathdup
char *aprintf(const char *fmt, ...)
{
char *result;
struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
strbuf_vaddf(sb, fmt, args);
va_end(args);
return strbuf_detach(sb);
}
or to have the caller pass in a pointer to strbuf instead of char *.
strbuf as argument is probably the right thing to do. I am doing that change.
The rest looks good to me.
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
[PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string
Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.
This will make it easier for callers to craft better error messages when
a transaction call fails.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 12 +++-
refs.h | 5 -
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 2d83ef6..0712912 100644
--- a/refs.c
+++ b/refs.c
@@ -3414,7 +3414,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, char **err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3424,6 +3425,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (!n)
return 0;
+ if (err)
+ *err = NULL;
+
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err) {
+ const char *str = Cannot lock the ref '%s'.;
+ *err = xmalloc(PATH_MAX + 24);
+ snprintf(*err, PATH_MAX + 24, str,
+update-refname);
+ }
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 892c5b6..615c73d 100644
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, char **err,
+ enum action_on_err onerr);
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
--
1.9.1.521.g5dc89fa
--
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 v3 03/19] refs.c: make ref_transaction_commit return an error string
Ronnie Sahlberg wrote:
Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.
s/returns/prints/?
This will make it easier for callers to craft better error messages when
a transaction call fails.
Interesting. Can you give an example? What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?
I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
-const char *msg, enum action_on_err onerr);
+const char *msg, char **err,
+enum action_on_err onerr);
Is the idea that if I pass in a pointer err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?
Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).
[...]
--- a/refs.c
+++ b/refs.c
[...]
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err) {
+ const char *str = Cannot lock the ref '%s'.;
+ *err = xmalloc(PATH_MAX + 24);
+ snprintf(*err, PATH_MAX + 24, str,
+ update-refname);
+ }
Might be clearer to use a helper similar to path.c::mkpathdup
char *aprintf(const char *fmt, ...)
{
char *result;
struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
strbuf_vaddf(sb, fmt, args);
va_end(args);
return strbuf_detach(sb);
}
or to have the caller pass in a pointer to strbuf instead of char *.
The rest looks good to me.
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
