Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref
On Wed, May 14, 2014 at 3:08 PM, Jonathan Nieder wrote:
> Hi,
>
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const
>> char *prefix)
> [...]
>> @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
>> char *prefix)
> [...]
>> if (delete || no_deref || argc > 0)
>> usage_with_options(git_update_ref_usage, options);
>> if (end_null)
>> line_termination = '\0';
>> update_refs_stdin();
>> - ret = ref_transaction_commit(transaction, msg, NULL,
>> - UPDATE_REFS_DIE_ON_ERR);
>> - return ret;
>> + if (ref_transaction_commit(transaction, msg, &err,
>> +UPDATE_REFS_QUIET_ON_ERR))
>> + die("%s", err.buf);
>
> Nice. I like this much more than passing a flag to each function to
> tell it how to handle errors. :)
>
> ref_transaction_commit didn't have any stray codepaths that return
> some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
> so this should be safe as far as the exit code is concerned.
>
> The only danger would be that some codepath leaves 'err' alone and
> forgets to write a messages, so we die with
>
> "fatal: "
>
> Alas, it looks like this patch can do that.
>
> i. The call to update_ref_write can error out without updating the
> error string.
Fixed.
I reordered the patches so the change to update_ref_write to take an
err argument will come before the change to update-ref.c as you
suggested.
>
> ii. delete_ref_loose can print a message and then fail without updating
> the error string so the output looks like
>
> warning: unable to unlink .git/refs/heads/master.lock: Permission
> denied
> fatal:
> $
>
Fixed.
I have added a new patch before the change to update-ref.c to add err
to delete_ref_loose.
> iii. repack_without_refs can similarly return an error
>
> error: Unable to create '/home/jrn/test/.git/packed-refs.lock:
> Permission denied
> error: cannot delete 'refs/heads/master' from packed refs
> fatal:
> $
>
> iv. commit_lock_file in commit_packed_refs is silent on error.
> repack_without_refs probably intends to write a message in that
> case but doesn't :(
Fixed.
I added a patch to take an err argument to repack_without_refs and
update it for both
conditions iii and iv.
>
> I wish there were some way to automatically detect missed spots or
> make them stand out (like with the current "return error()" idiom a
> bare "return -1" stands out).
>
> (i) is fixed by a later patch. It would be better to put that before
> this one for bisectability.
>
> I don't see fixes to (ii), (iii), and (iv) in the series yet from a
> quick glance.
Fixed in the next version of the patch series I will send out.
Thanks.
--
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 v6 05/42] update-ref.c: log transaction error from the update_ref
Hi,
Ronnie Sahlberg wrote:
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const
> char *prefix)
[...]
> @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
> char *prefix)
[...]
> if (delete || no_deref || argc > 0)
> usage_with_options(git_update_ref_usage, options);
> if (end_null)
> line_termination = '\0';
> update_refs_stdin();
> - ret = ref_transaction_commit(transaction, msg, NULL,
> - UPDATE_REFS_DIE_ON_ERR);
> - return ret;
> + if (ref_transaction_commit(transaction, msg, &err,
> +UPDATE_REFS_QUIET_ON_ERR))
> + die("%s", err.buf);
Nice. I like this much more than passing a flag to each function to
tell it how to handle errors. :)
ref_transaction_commit didn't have any stray codepaths that return
some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
so this should be safe as far as the exit code is concerned.
The only danger would be that some codepath leaves 'err' alone and
forgets to write a messages, so we die with
"fatal: "
Alas, it looks like this patch can do that.
i. The call to update_ref_write can error out without updating the
error string.
ii. delete_ref_loose can print a message and then fail without updating
the error string so the output looks like
warning: unable to unlink .git/refs/heads/master.lock: Permission denied
fatal:
$
iii. repack_without_refs can similarly return an error
error: Unable to create '/home/jrn/test/.git/packed-refs.lock:
Permission denied
error: cannot delete 'refs/heads/master' from packed refs
fatal:
$
iv. commit_lock_file in commit_packed_refs is silent on error.
repack_without_refs probably intends to write a message in that
case but doesn't :(
I wish there were some way to automatically detect missed spots or
make them stand out (like with the current "return error()" idiom a
bare "return -1" stands out).
(i) is fixed by a later patch. It would be better to put that before
this one for bisectability.
I don't see fixes to (ii), (iii), and (iv) in the series yet from a
quick glance.
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 v6 05/42] update-ref.c: log transaction error from the update_ref
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die("Refusing to perform update with empty message.");
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
- return ret;
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
+ return 0;
}
if (end_null)
--
2.0.0.rc1.351.g4d2c8e4
--
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
