From: Mark Jamsek Subject: Re: add diffstat option to got log and tog diff view To: Game of Trees Date: Wed, 4 Jan 2023 20:45:22 +1100 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68