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

Stefan Beller wrote:

This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.

[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html

The above is a useful kind of comment to put below the three-dashes.  It
doesn't explain what the intent behind the patch is, why I should want
this patch when considering whether to upgrade git, or what is going to
break when I consider reverting it as part of fixing something else, so
it doesn't belong in the commit message.

This patch doesn't intend any functional changes. It is just a refactoring, 
which replaces a char** array by a stringlist in the function 
repack_without_refs.

Thanks.  Why, though?  Is it about having something simpler to pass
from builtin/remote.c::remove_branches(), or something else?

Idea-by: Ronnie Sahlberg [off-list ref]
Signed-off-by: Stefan Beller <redacted>

Isn't the patch by Ronnie?

Sometimes I send a patch by someone else and make some change that I
don't want them to be blamed for.  Then I keep their sign-off and put
a note in the commit message about the change I made.  See output from

  git log origin/pu --grep='jc:'

for more examples of that.

Some nits below.

[...]

[...]

 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_insert(&delete_refs_list,
+				   states.stale.items[i].util);
+
+
 	if (states.stale.nr) {

(style) The double blank line looks odd here.

Now that there's no delete_refs array duplicating the string list,
would it make sense to rename delete_refs_list to delete_refs?

As a nice side-effect, that would make the definition of
delete_refs_list and other places it is used appear in the patch.

 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;

(optional) this could be

	for_each_string_list_item(ref, &delete_refs_list) {
		const char *refname = ref->string;
		...

which saves the reader from having to remember what states.stale.items
means.

[...]

[...]

[...]

-	int i, ret, removed = 0;
+	int count, ret, removed = 0;
 
 	assert(err);
 
-	/* Look for a packed ref */

The old code has comments marking sections of the function:

	/* Look for a packed ref */
	/* Avoid processing if we have nothing to do */
	/* Remove refnames from the cache */
	/* Remove any other accumulated cruft */
	/* Write what remains */

Is dropping this comment intended?

-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
-			break;
+	count = 0;
+	for_each_string_list_item(ref_to_delete, without)
+		if (get_packed_ref(ref_to_delete->string))
+			count++;

The old code breaks out early as soon as it finds a ref to delete.
Can we do similar?

E.g.

	for (i = 0; i < without->nr; i++)
		if (get_packed_ref(without->items[i].string))
			break;

(not about this patch) Is refs_to_delete leaked?

[...]

The old code doesn't xstrdup the list items, so _NODUP should work
fine (and be slightly more efficient).

[...]

string_list_append would be analagous to the old code.

[....]

A comment could mention whether the ref list needs to be sorted.  (It
doesn't, right?)

Thanks and hope that helps,
Jonathan