git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 18, 2014, 23:45 UTC
Message-ID
<20141118234500.GO6527@google.com>
In-Reply-To
<1416350636-12934-1-git-send-email-sbeller@google.com>
Stefan Beller wrote:
Show 6 quoted lines
> 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 <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
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.
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
[...]
> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
[...]
Show 9 quoted lines
>  	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.
Show 7 quoted lines
>  		printf_ln(_("Pruning %s"), remote);
>  		printf_ln(_("URL: %s"),
> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
>  		       ? states.remote->url[0]
>  		       : _("(no URL)"));
>  
> -		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));

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.

[...]
> +++ b/refs.c
[...]
> @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
[...]
Show 6 quoted lines
> -	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?
Show 7 quoted lines
> -	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?
[...]
Show 10 quoted lines
> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  int ref_transaction_commit(struct ref_transaction *transaction,
>  			   struct strbuf *err)
>  {
> -	int ret = 0, delnum = 0, i;
> -	const char **delnames;
> +	int ret = 0, i;
>  	int n = transaction->nr;
>  	struct ref_update **updates = transaction->updates;
> +	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;

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

[...]
Show 7 quoted lines
> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  			}
>  
>  			if (!(update->flags & REF_ISPRUNING))
> -				delnames[delnum++] = update->lock->ref_name;
> +				string_list_insert(&refs_to_delete,
> +						   update->lock->ref_name);
string_list_append would be analagous to the old code.
[....]
Show 9 quoted lines
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);

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

Thanks and hope that helps, Jonathan

Previous: Junio C HamanoNext: Stefan Beller
Message 4 of 61 in “refs.c: use a stringlist for repack_without_refs”
  1. refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 18, 2014
  2. Junio C HamanoNov 18, 2014
  3. Junio C HamanoNov 18, 2014
  4. Jonathan NiederNov 18, 2014
  5. Stefan BellerNov 19, 2014
  6. refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 19, 2014
  7. Junio C HamanoNov 19, 2014
  8. refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 19, 2014
  9. Jonathan NiederNov 19, 2014
  10. refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 19, 2014
  11. refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 19, 2014
  12. Jonathan NiederNov 20, 2014
  13. Junio C HamanoNov 20, 2014
  14. 1/1 refs.c: use a stringlist for repack_without_refsStefan Beller, Nov 20, 2014
  15. refs.c: repack_without_refs may be called without error string bufferStefan Beller, Nov 20, 2014
  16. Ronnie SahlbergNov 20, 2014
  17. Jonathan NiederNov 20, 2014
  18. Ronnie SahlbergNov 20, 2014
  19. Stefan BellerNov 20, 2014
  20. Jonathan NiederNov 20, 2014
  21. Jonathan NiederNov 20, 2014
  22. Junio C HamanoNov 20, 2014
  23. Stefan BellerNov 20, 2014
  24. refs.c: use a string_list for repack_without_refsStefan Beller, Nov 20, 2014
  25. Jonathan NiederNov 20, 2014
  26. 0/6 repack_without_refs(): convert to string_listMichael Haggerty, Nov 21, 2014
  27. 1/6 prune_remote(): exit early if there are no stale referencesMichael Haggerty, Nov 21, 2014
  28. Jonathan NiederNov 22, 2014
  29. 2/6 prune_remote(): initialize both delete_refs lists in a single loopMichael Haggerty, Nov 21, 2014
  30. 3/6 prune_remote(): sort delete_refs_list references en masseMichael Haggerty, Nov 21, 2014
  31. Junio C HamanoNov 21, 2014
  32. Michael HaggertyNov 25, 2014
  33. Michael HaggertyNov 25, 2014
  34. Jonathan NiederNov 22, 2014
  35. 4/6 repack_without_refs(): make the refnames argument a string_listMichael Haggerty, Nov 21, 2014
  36. Jonathan NiederNov 22, 2014
  37. Michael HaggertyNov 25, 2014
  38. 5/6 prune_remote(): rename local variableMichael Haggerty, Nov 21, 2014
  39. Jonathan NiederNov 22, 2014
  40. 6/6 prune_remote(): iterate using for_each_string_list_item()Michael Haggerty, Nov 21, 2014
  41. Jonathan NiederNov 22, 2014
  42. Michael HaggertyNov 21, 2014
  43. Junio C HamanoNov 21, 2014
  44. Stefan BellerNov 21, 2014
  45. Our cumbersome mailing list workflow (was: Re: [PATCH 0/6] repack_without_refs(): convert to string_list)Michael Haggerty, Nov 25, 2014
  46. Torsten BögershausenNov 27, 2014
  47. Matthieu MoyNov 27, 2014
  48. Philip OakleyNov 28, 2014
  49. Eric WongNov 27, 2014
  50. Michael HaggertyNov 28, 2014
  51. brian m. carlsonNov 28, 2014
  52. Junio C HamanoDec 1, 2014
  53. Stefan BellerDec 3, 2014
  54. Jonathan NiederDec 3, 2014
  55. Junio C HamanoDec 3, 2014
  56. Torsten BögershausenDec 3, 2014
  57. Michael HaggertyNov 28, 2014
  58. Marc BranchaudNov 28, 2014
  59. Damien RobertNov 28, 2014
  60. Philip OakleyDec 3, 2014
  61. Stefan BellerDec 4, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.