On Fri, Jul 18, 2014 at 3:59 PM, Junio C Hamano <gitster@xxxxxxxxx> 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 <gitster@xxxxxxxxx> wrote:
>> Ronnie Sahlberg <sahlberg@xxxxxxxxxx> writes:
>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>>> ---
>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html