On Tue, Jul 8, 2014 at 7:19 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
>> Add an err argument to delete_loose_ref so that we can pass a descriptive
>> error string back to the caller. Pass the err argument from transaction
>> commit to this function so that transaction users will have a nice error
>> string if the transaction failed due to delete_loose_ref.
>>
>> Add a new function unlink_or_err that we can call from delete_ref_loose. This
>> function is similar to unlink_or_warn except that we can pass it an err
>> argument. If err is non-NULL the function will populate err instead of
>> printing a warning().
>>
>> Simplify warn_if_unremovable.
>
> The change to warn_if_unremovable() is orthogonal to the rest of the
> commit and should be a separate commit.
Done.
I split it into three patches.
One patch for the warn_if_removable change
another patch to add unlink_or_msg to wrapper.c
and a final patch to change delete_loose_ref.
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>> ---
>> refs.c | 33 ++++++++++++++++++++++++++++-----
>> wrapper.c | 14 ++++++--------
>> 2 files changed, 34 insertions(+), 13 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 92a06d4..c7d1f3e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2544,16 +2544,38 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>> return ret;
>> }
>>
>> -static int delete_ref_loose(struct ref_lock *lock, int flag)
>> +static int add_err_if_unremovable(const char *op, const char *file,
>> + struct strbuf *e, int rc)
>
> This function is only used once. Given also that its purpose is not
> that obvious from its signature, it seems to me that the code would be
> easier to read if it were inlined.
>
>> +{
>> + int err = errno;
>> + if (rc < 0 && errno != ENOENT) {
>> + strbuf_addf(e, "unable to %s %s: %s",
>> + op, file, strerror(errno));
>> + errno = err;
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> +static int unlink_or_err(const char *file, struct strbuf *err)
>
> The name of this function is misleading; it sounds like it will try to
> unlink the file and if not possible call error(). Maybe a name like
> "unlink_or_report" would be less prejudicial.
>
> It might also make sense to move this function to wrapper.c and
> implement unlink_or_warn() in terms of it rather than vice versa.
>
>> +{
>> + if (err)
>> + return add_err_if_unremovable("unlink", file, err,
>> + unlink(file));
>> + else
>> + return unlink_or_warn(file);
>> +}
>> +
>> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>> {
>> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>> /* loose */
>> - int err, i = strlen(lock->lk->filename) - 5; /* .lock */
>> + int res, i = strlen(lock->lk->filename) - 5; /* .lock */
>>
>> lock->lk->filename[i] = 0;
>> - err = unlink_or_warn(lock->lk->filename);
>> + res = unlink_or_err(lock->lk->filename, err);
>> lock->lk->filename[i] = '.';
>> - if (err && errno != ENOENT)
>> + if (res)
>> return 1;
>> }
>> return 0;
>> @@ -3603,7 +3625,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>> struct ref_update *update = updates[i];
>>
>> if (update->lock) {
>> - ret |= delete_ref_loose(update->lock, update->type);
>> + ret |= delete_ref_loose(update->lock, update->type,
>> + err);
>> if (!(update->flags & REF_ISPRUNING))
>> delnames[delnum++] = update->lock->ref_name;
>> }
>> diff --git a/wrapper.c b/wrapper.c
>> index bc1bfb8..740e193 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
>>
>> static int warn_if_unremovable(const char *op, const char *file, int rc)
>> {
>> - if (rc < 0) {
>> - int err = errno;
>> - if (ENOENT != err) {
>> - warning("unable to %s %s: %s",
>> - op, file, strerror(errno));
>> - errno = err;
>> - }
>> - }
>> + int err;
>> + if (rc >= 0 || errno == ENOENT)
>> + return rc;
>> + err = errno;
>> + warning("unable to %s %s: %s", op, file, strerror(errno));
>> + errno = err;
>> return rc;
>> }
>>
>>
>
> Michael
>
> --
> Michael Haggerty
> mhagger@xxxxxxxxxxxx
>
--
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