On Fri, Jul 18, 2014 at 3:59 PM, Junio C Hamano <gits...@pobox.com> wrote:
> 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?

Not necessarily.
I can think of reasons both for and against "abort on first failure".

One reason for the former could be if there are problems with multiple
refs in a single transaction.
It would be very annoying to have to do
$ git <some command>
   error: ref foo has a problem

$ <run command to fix the problem>
$ git <some sommand>     (try again)
   error: ref bar has a problem
...

And it might be more userfriendly if that instead would be
$ git <some command>
   error: ref foo has a problem
   error: ref bar has a problem
   ...

And get all the failures in one go instead of having to iterate.

The reason for the latter I think is it would be cleaner/simpler/...
to just have an "abort on first failure".


On the past discussions on the list I think I have heard voices for
both approaches.
I don't think we have all that many
multiple-refs-in-a-single-transaction yet in what is checked in so far
so I think we are practically still "abort on first error".

I personally do not know yet which approach is the best but would like
to keep the door open for the "try all and fail at the end".
That said, I do not feel all that strongly about this.
If you have strong feelings about this I can remove the unlink_or_msg
patch and rework the rest of the series to cope with it.


regards
ronnie sahlberg



>
> I dunno...
>
>
> On Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano <gits...@pobox.com> wrote:
>> Ronnie Sahlberg <sahlb...@google.com> writes:
>>
>>> Signed-off-by: Ronnie Sahlberg <sahlb...@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));
--
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

Reply via email to