Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg
- From
Ronnie Sahlberg <sahlberg@google.com>
- Date
- Jul 22, 2014, 17:42 UTC
- Message-ID
- <CAL=YDWmHcy+Kf+gJLHyFK7bVjnD+bk7rX22jqHVFmTXoHDCEhQ@mail.gmail.com>
- In-Reply-To
- <CAPc5daW_6bVg4B4GHA-HCRL7bzmLAdVOF2xOYa9aOOjze-zTdA@mail.gmail.com>
On Fri, Jul 18, 2014 at 3:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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
Show 79 quoted lines
>
> 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));