On Wed, Jan 04, 2023 at 05:43:57PM +1100, Mark Jamsek wrote:
> On 23-01-03 08:01PM, Stefan Sperling wrote:
> > I would suggest to always turn this on in tog, replacing the -P style
> > path list display. And leave it off by default on the command line.
> 
> I like this idea! The change is all but imperceptible for most diffs, and
> it adds useful context when reviewing commits.
> 
> The below diff is basically the previous except it's now turned on in
> tog; and if I've understood your suggestion correctly, I've removed the
> keymap to toggle it on/off. Let me know if I've misunderstood and I'll
> add the knob back.

Yes, this is what I had in mind. Thanks!
A toggle won't be needed unless the perceived performance burden turns
out to be too high and to be insurmountable.

> > If this becomes a burdensome performance problem, we can look at ways
> > to speed it up or disable it by default. Keeping diff_result around is
> > a possible optimization that has been mentioned before.
> > Our current high-level diff APIs don't expose the diff_result, which
> > is suboptimal. We could add an extra set of API which expose it, or
> > change the existing APIs to follow the computation/output split that
> > is present in the diff.git API.
> 
> I think it's a good idea to expose the diff_result; although, apart from
> this case, I don't have any other immediate ideas where we might want to
> reuse it. But even in this case it would be a definite plus.

We currently recompute the entire diff when the '[' and ']' keys are used
in the tog diff view to reduce/increase the amount of context lines.
Context lines are produced by the unidiff output routines, therefore using
the same diff_result should improve the responsiveness of these keys.
 
> @@ -4673,6 +4714,24 @@ done:
>  	fputc('\n', outfile);
>  	outoff++;
>  	err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE);
> +	if (err)
> +		goto done;
> +
> +	n = fprintf(outfile,
> +	    "%d file%s changed, %d insertions(+), %d deletions(-)\n",
> +	    dsa.nfiles, dsa.nfiles > 1 ? "s" : "", dsa.ins, dsa.del);
> +	if (n < 0) {
> +		err = got_error_from_errno("fprintf");
> +		goto done;
> +	}
> +	outoff += n;
> +	err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE);
> +	if (err)
> +		goto done;
> +
> +	fputc('\n', outfile);
> +	outoff++;
> +	err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE);
>  done:
>  	got_pathlist_free(&changed_paths);

It seems memory for the allocated diffstat changes is leaked here
because got_pathlist_free() will not free the 'data' pointer. This is
because some callers store pointers in 'data' that cannot be freed.
The above code should iterate the pathlist and free pe->data manually
before freeing the list itself.