From: James Cook <falsifian@falsifian.org>
Subject: Re: in diff error message, say what was being diffed
To: gameoftrees@openbsd.org
Date: Mon, 18 Sep 2023 14:47:17 +0000

On Mon, Sep 18, 2023 at 08:47:31AM +0200, Stefan Sperling wrote:
>On Mon, Sep 18, 2023 at 01:11:22AM +0000, James Cook wrote:
>> On Sat, Sep 16, 2023 at 08:16:34PM +0200, Stefan Sperling wrote:
>> > Could you use got_error_fmt() instead of snprintf + got_error_msg?
>>
>> If I switch to got_error_fmt, then instead of this
>> 	got: /dev/null vs /g: diff_atomize_file: Cannot allocate memory
>> I get
>> 	got: /dev/null vs /g: diff_atomize_file: Cannot allocate memory: see errno
>>
>> The trouble is got_error_fmt takes responsibility for appending a string
>> describing the error code, which is not useful in this case.
>
>Oh, indeed. Sorry about that.
>
>> One option would be to add a function to the error library that "wraps" an
>> error by prepending a string. E.g.
>>
>> const struct got_error *got_error_prepend_msg(
>>     const struct got_error*, const char *);
>>
>> I don't know if there are other places where that would come in handy.
>
>There is another function, called got_error_from_errno_fmt().
>Would that one work?
>In any case, I would rather tweak the existing error interfaces to
>match our needs than add even more variants.

The trouble with the from_errno variants is that at this point in 
dif_blob_file(), I've just got a generic struct got_error to work 
with, as returned by got_diffreg(). It's possible errno still has 
the original error in it, but even if it does, err->msg at this point 
contains
	diff_atomize_file: Cannot allocate memory
and re-generating the error message will etiher lose the 
"diff_atomize_file:" part (if we ignore the existing err->msg) or 
duplicate the "Cannot allocate memory" part (if we wrap the existing 
err->msg and use one of the from_errno functions.

This may be a sign that I'm deviating too much from existing coding 
style. I haven't seen any other places where an existing error 
message is "wrapped". I suppose another option would be to pass the
error context to the got_diffreg() function, but that seems like 
overcomplicating things.

Another possibility is to just drop the patch to avoid complicating 
things.

>> To be precise, the new diff I tried that gave the extra "see errno" was:
>>
>> --BEGIN--
>> --- lib/diff.c
>> +++ lib/diff.c
>> @@ -378,8 +378,12 @@ diff_blob_file(struct got_diffreg_result **resultp,
>>         err = got_diffreg(&result, f1, f2, diff_algo, ignore_whitespace,
>>             force_text_diff);
>> -       if (err)
>> -               goto done;
>> +       if (err) {
>> +               err = got_error_fmt(err->code, "%s vs %s: %s",
>> +                           label1 ? label1 : idstr1,
>> +                           f2_exists ? label2 : "/dev/null", err->msg);
>> +                goto done;
>> +       }
>>         if (outfile) {
>>                 err = got_diffreg_output(NULL, NULL, result,
>> --END--
>

-- 
James