On 23-01-04 10:05AM, Stefan Sperling wrote:
> 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.

No worries! I haven't found the performance cost a problem yet. Granted
it's been a very short time and my perceptions and tolerances may differ
from others but, as discussed, if it becomes a problem we already have
a potential solution. And, like you say, failing that we can add the
toggle back.

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

Yes, good example! That's another definite improvement. I'm liking the
idea of exposing diff_result even more. I'm taking a break from renos
and my sister is taking the kids for a short vacation so I'm hoping to
get some hacking in before uni resumes :)

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

We already free pe->data above in the queue loop where we write out the
changeset; line 4711 with the patch applied:

	4794	TAILQ_FOREACH(pe, &changed_paths, entry) {
	4795		struct got_diff_changed_path *cp = pe->data;
	4796		int pad = dsa.max_path_len - pe->path_len + 1;

	...		...

	4710		free((char *)pe->path);
	4711		free(pe->data);
	4712	}

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68