Re: [PATCH] for-each-ref: delay parsing of --sort=<atom> options

On Tue, Oct 19, 2021 at 06:18:40PM -0400, Jeff King wrote:

quoted

@@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("ls-remote");
 
-	UNLEAK(sorting);
-
 	if (argc > 1) {
 		int i;
 		CALLOC_ARRAY(pattern, argc);

@@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		item->symref = xstrdup_or_null(ref->symref);
 	}
 
-	if (sorting)
+	if (sorting_options.nr) {
+		struct ref_sorting *sorting;
+		UNLEAK(sorting);
+
+		sorting = ref_sorting_options(&sorting_options);
 		ref_array_sort(sorting, &ref_array);
+	}

I wondered at first about pulling this UNLEAK() down, but it's because
you move the "sorting" variable itself into the smaller scope. So this
makes sense (and calling UNLEAK() before the pointer is set is perfectly
fine, since it takes the address of the auto variable). It is a shame
you can't just ref_sorting_free() afterwards, but we don't have that
function yet. And adding it is way out of scope here. :)

Actually, I think I was wrong here. UNLEAK() will look at &sorting, but
it will snapshot its data at the time of the call. So it won't do
anything when the variable doesn't yet have a value.

You can demonstrate with:

  $ make SANITIZE=leak
  $ ./git ls-remote --sort=refname .

which will complain. Bumping it down like this:

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1e6017cdaa..a94a220256 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c

@@ -139,10 +139,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	if (sorting_options.nr) {
 		struct ref_sorting *sorting;
-		UNLEAK(sorting);
 
 		sorting = ref_sorting_options(&sorting_options);
 		ref_array_sort(sorting, &ref_array);
+		UNLEAK(sorting);
 	}
 
 	for (i = 0; i < ref_array.nr; i++) {

clears it up. Note that there are other similar "leaks" (e.g., if you
give a pattern in argv[1]) which should be punted to another topic, but
I think you'd want to deal with this one since you're moving the
UNLEAK() around.

-Peff