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 01:11:22 +0000

On Sat, Sep 16, 2023 at 08:16:34PM +0200, Stefan Sperling wrote:
>On Sat, Sep 16, 2023 at 04:22:36PM +0000, James Cook wrote:
>> Recently I ran "got ci" to add a large number (>100 000) files and got the
>> error:
>> 	got: diff_atomize_file: Cannot allocate memory
>>
>> It turns out one of the files was really big. This diff changes the error to
>> say:
>> 	got: /dev/null vs /path/to/the/really/big/file: diff_atomize_file: Cannot allocate memory
>>
>> Maybe it's to verbose and/or unneccesary. Even with the original error it
>> was not hard to guess that "got ci -n" would fix the problem. However, this
>> error would have saved me a bit of time because I ended up trying to
>> understand why adding a large number of files
>> would trigger an allocation error (memory leak?), and only deep into
>> debugging did I realize it was one big file causing the problem.
>>
>> --
>> James
>>
>>
>> diff /home/falsifian/co/got
>> commit - 19a5edf3da4acec32a964ac91c065e121b0a0ec2
>> path + /home/falsifian/co/got
>> blob - f448440ff5880b5d6ec7ec9b7d45ef85b47ee2e6
>> file + lib/diff.c
>> --- lib/diff.c
>> +++ lib/diff.c
>> @@ -378,8 +378,15 @@ diff_blob_file(struct got_diffreg_result **resultp,
>>  	err = got_diffreg(&result, f1, f2, diff_algo, ignore_whitespace,
>>  	    force_text_diff);
>> -	if (err)
>> +	if (err) {
>> +		char msg[GOT_ERR_MAX_MSG_SIZE];
>> +		if (snprintf(msg, sizeof(msg), "%s vs %s: %s",
>> +		             label1 ? label1 : idstr1,
>> +		             f2_exists ? label2 : "/dev/null", err->msg) >= 0) {
>> +			err = got_error_msg(err->code, msg);
>> +		}
>>  		goto done;
>> +	}
>
>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.

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.

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--


>I am happy with this change. As a follow-up change, should we try to detect
>this ENOMEM condition in got commit and proceed without showing a diff?
>
>>  	if (outfile) {
>>  		err = got_diffreg_output(NULL, NULL, result,

Yes, that sounds useful. I may or may not have time for this; my 
schedule is hard to predict.

-- 
James