Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 14, 2014 at 3:08 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]