Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose
On Tue, May 27, 2014 at 5:25 PM, Jonathan Nieder wrote:
> Hi,
>
> Comments from http://marc.info/?l=git&m=140079653930751&w=2:
>
> Ronnie Sahlberg wrote:
>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
>> return repack_without_refs(&refname, 1, NULL);
>> }
>>
>> -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)
>> +{
>> + int err = errno;
>> + if (rc < 0 && err != ENOENT) {
>> + strbuf_addf(e, "unable to %s %s: %s",
>> + op, file, strerror(errno));
>> + errno = err;
>> + }
>> + return rc;
>> +}
>> +
>> +static int unlink_or_err(const char *file, struct strbuf *err)
>> +{
>> + if (err)
>> + return add_err_if_unremovable("unlink", file, err,
>> + unlink(file));
>> + else
>> + return unlink_or_warn(file);
>> +}
>
> The new unlink_or_err has an odd contract when the err argument is passed.
> On error:
>
> * if errno != ENOENT, it will append a message to err and return -1 (good)
> * if errno == ENOENT, it will not append a message to err but will
>still return -1.
>
> This means the caller has to check errno to figure out whether err is
> meaningful when it returns an error.
>
> Perhaps it should return 0 when errno == ENOENT. After all, in that
> case the file does not exist any more, which is all we wanted.
Good idea.
Thanks. Done.
>
>
>> +
>> +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 && errno != ENOENT) {
>> + if (err)
>> + strbuf_addf(err,
>> + "failed to delete loose ref '%s'",
>> + lock->lk->filename);
>
> On failure we seem to add our own message to err, too, so the
> resulting message would be something like
>
> fatal: unable to unlink .git/refs/heads/master: \
> Permission deniedfailed to delete loose ref
> '.git/refs/heads/master.lock'
>
> Is the second strbuf_addf a typo?
Yeah, we don't need it.
Removed.
>
> 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 v11 08/41] refs.c: add an err argument to delete_ref_loose
Hi,
Comments from http://marc.info/?l=git&m=140079653930751&w=2:
Ronnie Sahlberg wrote:
[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
> return repack_without_refs(&refname, 1, NULL);
> }
>
> -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)
> +{
> + int err = errno;
> + if (rc < 0 && err != ENOENT) {
> + strbuf_addf(e, "unable to %s %s: %s",
> + op, file, strerror(errno));
> + errno = err;
> + }
> + return rc;
> +}
> +
> +static int unlink_or_err(const char *file, struct strbuf *err)
> +{
> + if (err)
> + return add_err_if_unremovable("unlink", file, err,
> + unlink(file));
> + else
> + return unlink_or_warn(file);
> +}
The new unlink_or_err has an odd contract when the err argument is passed.
On error:
* if errno != ENOENT, it will append a message to err and return -1 (good)
* if errno == ENOENT, it will not append a message to err but will
still return -1.
This means the caller has to check errno to figure out whether err is
meaningful when it returns an error.
Perhaps it should return 0 when errno == ENOENT. After all, in that
case the file does not exist any more, which is all we wanted.
> +
> +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 && errno != ENOENT) {
> + if (err)
> + strbuf_addf(err,
> + "failed to delete loose ref '%s'",
> + lock->lk->filename);
On failure we seem to add our own message to err, too, so the
resulting message would be something like
fatal: unable to unlink .git/refs/heads/master: \
Permission deniedfailed to delete loose ref
'.git/refs/heads/master.lock'
Is the second strbuf_addf a typo?
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 v11 08/41] refs.c: add an err argument to delete_ref_loose
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().
Signed-off-by: Ronnie Sahlberg
---
refs.c | 39 +--
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 4c7f4f7..891b80c 100644
--- a/refs.c
+++ b/refs.c
@@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(&refname, 1, NULL);
}
-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)
+{
+ int err = errno;
+ if (rc < 0 && err != ENOENT) {
+ strbuf_addf(e, "unable to %s %s: %s",
+ op, file, strerror(errno));
+ errno = err;
+ }
+ return rc;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ 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 && errno != ENOENT) {
+ if (err)
+ strbuf_addf(err,
+ "failed to delete loose ref '%s'",
+ lock->lk->filename);
return 1;
+ }
}
return 0;
}
@@ -2514,7 +2540,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
if (!lock)
return 1;
- ret |= delete_ref_loose(lock, flag);
+ ret |= delete_ref_loose(lock, flag, NULL);
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -3494,7 +3520,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
- ret |= delete_ref_loose(update->lock, update->type);
+ ret |= delete_ref_loose(update->lock, update->type,
+ err);
}
}
--
2.0.0.rc3.474.g0203784
--
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
