Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs

Jonathan Nieder [off-list ref] writes:

[...]

quoted

+++ b/builtin/remote.c

[...]

quoted

@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	sort_string_list(&delete_refs);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
+	string_list_clear(&delete_refs, 0);
 
 	free_remote_ref_states(&states);
 	return result;

Micronit: it would be clearer (and easier to remember to free the list
in other code paths if this function gains more 'return' statements)
with the string_list_clear in the same block as other code that frees
resources (i.e., if the blank line moved one line up).

Thanks for a careful reading.  This kind of attention to detail
helps the longer term health of the codebase.

The function has to be able to write to 'err' on error, so I think the
comment doesn't have to mention that err must be non-NULL.  Any caller
that tries to pass NULL will get an assertion error quickly.

That invites a bit of question, though.

An equally plausible alternative definition for set of API functions
that take strbuf *err is to pass it only when you care about the
explanation of the error (i.e. it is valid for "git cmd --quiet" to
pass NULL there) [*1*] (do we already have such a function?).  And
the comment may help clarifying which is which.  I however think we
shouldn't have mixtures (formatting into "strbuf *err" may be costly
when we know we are asked to fail silently, but an error path is not
usually performance sensitive).


[Footnote]

*1* With yet another one, a function may call error() on its own
when a NULL is passed to strbuf *err, but let's not go there.