From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got: add diffstat to got diff
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Sun, 8 Jan 2023 17:44:55 +0100

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?

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