From: Mark Jamsek <mark@jamsek.com>
Subject: Re: got: add diffstat to got diff
To: Game of Trees <gameoftrees@openbsd.org>
Date: Mon, 9 Jan 2023 19:19:55 +1100

On 23-01-08 05:44PM, Stefan Sperling wrote:
> On Mon, Jan 09, 2023 at 03:11:01AM +1100, Mark Jamsek wrote:
> > +static void
> > +print_diffstat(struct got_diffstat_cb_arg *dsa, struct got_pathlist_head *paths,
> > +    int newline)
> > +{
> > +	struct got_pathlist_entry *pe;
> > +
> > +	TAILQ_FOREACH(pe, paths, entry) {
> > +		struct got_diff_changed_path *cp = pe->data;
> > +		int pad = dsa->max_path_len - pe->path_len + 1;
> > +
> > +		printf(" %c  %s%*c | %*d+ %*d-\n", cp->status, pe->path, pad,
> > +		    ' ', dsa->add_cols + 1, cp->add, dsa->rm_cols + 1, cp->rm);
> > +
> > +		free(pe->data);
> > +		free((char *)pe->path);
> > +	}
> > +	if (newline)
> > +		printf("\n");
> > +	printf("%d file%s changed, %d insertions(+), %d deletions(-)\n",
> > +	    dsa->nfiles, dsa->nfiles > 1 ? "s" : "", dsa->ins, dsa->del);
> > +	if (newline)
> > +		printf("\n");
> > +
> > +	got_pathlist_free(paths);
> 
> The above call to got_pathlist_free seems redundant?

Hmm...I'm not sure why I've put that there!

Actually, yes I do. I lifted that routine from where I originally put
it: inline at the end of cmd_diff(). Rather than dup the code because we
output the diffstat in a couple places, I wrapped it in a function and
forgot to remove the copypasta'd got_pathlist_free() call after putting
one at the end of cmd_diff()--I don't like deallocating in a separate
function from where the resource is allocated. IOW, yes, it's redundant.
Thanks :)

> It seems that print_commits and diff_main already take care of freeing
> this list. It's a bit hard to follow how print_commits() does this,
> but diff_main is straightforward and frees diffstat_paths below its
> "done:" label, though it forgets about freeing the data pointers.
> 
> I think we should avoid freeing the data while displaying it. This is not
> a great coding pattern because it becomes harder to judge the lifetime of
> these allocations, and there is no benefit in deallocating this data early.
> The above code is not even setting pe->data and pe->path to NULL after
> freeing, so a double-free is just waiting to happen.
> 
> Looping the list once again to free all pointers takes a small bit
> of time but we should make it as easy as possible to keep track of
> allocation lifetimes in our minds (since we are writing in C...)

tbh, I would prefer that too. I shamelessly copied the existing
print-and-free idiom used elsewhere when outputting the changed
pathlist.  We free entry members often enough that I would even like
either a boolean in the existing got_pathlist_free() routine to
optionally free members, or perhaps another got_pathlist_free_all()
routine that also frees each entry's members; then we could save the
extra loop. If you think that's worthwhile, that would be another diff
though. For now I'll drop the redundant call and do another loop.

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