From: Stefan Sperling <stsp@stsp.name>
Subject: Re: refactor got_pathlist_free() to optionally free path and data pointers
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Mon, 9 Jan 2023 15:53:00 +0100

On Tue, Jan 10, 2023 at 01:28:23AM +1100, Mark Jamsek wrote:
> As briefly discussed with stsp, refactor got_pathlist_free() to
> optionally free member pointers.
> 
> Although this touches a lot of code, the change is quite simple.
> According to grep I haven't missed anything, but there's always
> a non-zero chance I haven't got the translation right in every case!

I went through and did spot one conversion problem. See below.

> blob - 7cb114b362ef4f4a6d3f385fb37066d5a97691bd
> blob + 9eb5ed8033a8621005debb45847be7cb1fb7598a
> --- include/got_path.h
> +++ include/got_path.h
> @@ -93,7 +93,10 @@ void got_pathlist_free(struct got_pathlist_head *);
>      const char *, void *);
>  
>  /* Free resources allocated for a path list. */
> -void got_pathlist_free(struct got_pathlist_head *);
> +#define PATHLIST_FREE_NONE	0

Please always add a GOT_ prefix to macros in our headers if possible.
Use GOT_PATHLIST_FREE_NONE, etc.

> +#define PATHLIST_FREE_PATH	1 << 1

Missing parentheses, and should start on bit index 0, not bit index 1.

#define GOT_PATHLIST_FREE_PATH	(1 << 0)

> +#define PATHLIST_FREE_DATA	1 << 2

#define GOT_PATHLIST_FREE_DATA	(1 << 1)

> @@ -6240,11 +6230,7 @@ done:
>  	unlockerr = lock_worktree(worktree, LOCK_SH);
>  	if (unlockerr && err == NULL)
>  		err = unlockerr;
> -	TAILQ_FOREACH(pe, &commitable_paths, entry) {
> -		struct got_commitable *ct = pe->data;
> -		free_commitable(ct);
> -	}
> -	got_pathlist_free(&commitable_paths);
> +	got_pathlist_free(&commitable_paths, PATHLIST_FREE_DATA);

free_commitable() calls free() multiple times, so unfortunately this
code cannot be converted to the new API. I would keep this as-is, and
call got_pathlist_free with GOT_PATHLIST_FREE_NONE.

>  static const struct got_error *
>  store_commit_id(const char *commit_ref_name, struct got_object_id *commit_id,
>      int is_rebase, struct got_repository *repo)
> @@ -7955,11 +7930,7 @@ done:
>  	if (sync_err && err == NULL)
>  		err = sync_err;
>  done:
> -	TAILQ_FOREACH(pe, &commitable_paths, entry) {
> -		struct got_commitable *ct = pe->data;
> -		free_commitable(ct);
> -	}
> -	got_pathlist_free(&commitable_paths);
> +	got_pathlist_free(&commitable_paths, PATHLIST_FREE_DATA);

Same.