Re: [PATCH 06/11] branch: fix a leak in cmd_branch

[Jeff King <peff@xxxxxxxx>]

On Sun, Jun 11, 2023 at 08:50:05PM +0200, Rubén Justo wrote:

> We don't have a common clean-up section in cmd_branch().  To avoid
> refactoring and keep the fix simple, and while we find a better
> solution, let's silence the leak-hunter making the list static.

Gross. :)

If we are just going to annotate here, I'd prefer to use UNLEAK(). It
makes it more obvious this is about leak-checking, and doesn't imply
that we otherwise care about the lifetime of this field. Like:

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..075e580d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	UNLEAK(sorting_options);
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));

> diff --git a/builtin/branch.c b/builtin/branch.c
> index e6c2655af6..759480fe8d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -709,7 +709,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	enum branch_track track;
>  	struct ref_filter filter;
>  	static struct ref_sorting *sorting;
> -	struct string_list sorting_options = STRING_LIST_INIT_DUP;
> +	static struct string_list sorting_options = STRING_LIST_INIT_DUP;

It's interesting that the ref_sorting pointer is also static. This goes
back to aedcb7dc75 (branch.c: use 'ref-filter' APIs, 2015-09-23). I
wonder if it was trying to accomplish the same thing (back then we added
directly to the list), though back then we did not have any real
leak-detection tools set up.

It should probably move into the "if (list)" block, but that is
orthogonal to your patch.

As for the more complete fix, I think we'd perhaps want something like
this, which would also detect when --sort is used in a non-list mode (as
your example shows, it's a little complicated because we stuff
configured entries into the same list):

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..bcd2d9e1d9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -708,8 +708,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    recurse_submodules_explicit = 0;
 	enum branch_track track;
 	struct ref_filter filter;
-	static struct ref_sorting *sorting;
 	struct string_list sorting_options = STRING_LIST_INIT_DUP;
+	size_t nr_configured_sorting_options;
 	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option options[] = {
@@ -773,6 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_branch_usage, options);
 
 	git_config(git_branch_config, &sorting_options);
+	nr_configured_sorting_options = sorting_options.nr;
 
 	track = git_branch_track;
 
@@ -832,6 +833,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	if (!list) {
+		if (sorting_options.nr != nr_configured_sorting_options)
+			die(_("--sort does not make sense when not listing"));
+		string_list_clear(&sorting_options, 0);
+	}
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
@@ -840,6 +847,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		print_current_branch_name();
 		return 0;
 	} else if (list) {
+		struct ref_sorting *sorting;
 		/*  git branch --list also shows HEAD when it is detached */
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
 			filter.kind |= FILTER_REFS_DETACHED_HEAD;

-Peff