Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

[Junio C Hamano <gitster@pobox.com>]

Hmm, the primary reason for this seems to be because you are going to handle
multiple refs at a time, some of them might fail to lock due to this
lowest-level
helper to unlink failing, some others may fail to lock due to some other reason,
and the user may want to be able to differentiate various modes of failure.

But even if that were the case, would it be necessary to buffer the messages
like this and give them all at the end? In the transaction code path,
it is likely
that you would be aborting the whole thing at the first failure, no?

I dunno...


On Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  git-compat-util.h |  6 ++++++
>>  wrapper.c         | 18 ++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index b6f03b3..426bc98 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>>  #endif
>>  #endif
>>
>> +#include "strbuf.h"
>> +
>>  /*
>>   * Preserves errno, prints a message, but gives no warning for ENOENT.
>>   * Always returns the return value of unlink(2).
>>   */
>>  int unlink_or_warn(const char *path);
>>  /*
>> + * Like unlink_or_warn but populates a strbuf
>> + */
>> +int unlink_or_msg(const char *file, struct strbuf *err);
>> +/*
>>   * Likewise for rmdir(2).
>>   */
>>  int rmdir_or_warn(const char *path);
>> diff --git a/wrapper.c b/wrapper.c
>> index 740e193..74a0cc0 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
>>       return rc;
>>  }
>>
>> +int unlink_or_msg(const char *file, struct strbuf *err)
>> +{
>> +     if (err) {
>> +             int rc = unlink(file);
>> +             int save_errno = errno;
>> +
>> +             if (rc < 0 && errno != ENOENT) {
>> +                     strbuf_addf(err, "unable to unlink %s: %s",
>> +                                 file, strerror(errno));
>> +                     errno = save_errno;
>> +                     return -1;
>> +             }
>> +             return 0;
>> +     }
>> +
>> +     return unlink_or_warn(file);
>> +}
>
> In general, I do not generally like to see messages propagated
> upwards from deeper levels of the callchain to the callers to be
> used later, primarily because that will easily make it harder to
> localize the message-lego.
>
> For this partcular one, shouldn't the caller be doing
>
>         if (unlink(file) && errno != ENOENT) {
>                 ... do its own error message ...
>         }
>
> instead of calling any of the unlink_or_whatever() helper?
>
>
>>  int unlink_or_warn(const char *file)
>>  {
>>       return warn_if_unremovable("unlink", file, unlink(file));