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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> 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.
>
> 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.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> Idea-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/remote.c | 22 +++++++---------------
>  refs.c           | 41 ++++++++++++++++++++---------------------
>  refs.h           |  3 +--
>  3 files changed, 28 insertions(+), 38 deletions(-)

In one codepath we were already using a string_list delete_refs_list
anyway, so it makes sense to reuse that by movingan existing call to
string_list_insert() a bit higher, instead of maintaining another
array of pointers delete_refs[] to strings.

OK, it simplifies the code by reducing the line count, which is a
plus ;-)

Sounds good.

>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 7f28f92..dca4ebf 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
>  static int remove_branches(struct string_list *branches)
>  {
>  	struct strbuf err = STRBUF_INIT;
> -	const char **branch_names;
>  	int i, result = 0;
>  
> -	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
> -	for (i = 0; i < branches->nr; i++)
> -		branch_names[i] = branches->items[i].string;
> -	if (repack_without_refs(branch_names, branches->nr, &err))
> +	if (repack_without_refs(branches, &err))
>  		result |= error("%s", err.buf);
>  	strbuf_release(&err);
> -	free(branch_names);
>  
>  	for (i = 0; i < branches->nr; i++) {
>  		struct string_list_item *item = branches->items + i;
> @@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
>  	int result = 0, i;
>  	struct ref_states states;
>  	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
> -	const char **delete_refs;
>  	const char *dangling_msg = dry_run
>  		? _(" %s will become dangling!")
>  		: _(" %s has become dangling!");
> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
>  	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) {
>  		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));
> -		for (i = 0; i < states.stale.nr; i++)
> -			delete_refs[i] = states.stale.items[i].util;
>  		if (!dry_run) {
>  			struct strbuf err = STRBUF_INIT;
> -			if (repack_without_refs(delete_refs, states.stale.nr,
> -						&err))
> +			if (repack_without_refs(&delete_refs_list, &err))
>  				result |= error("%s", err.buf);
>  			strbuf_release(&err);
>  		}
> -		free(delete_refs);
>  	}
>  
>  	for (i = 0; i < states.stale.nr; i++) {
>  		const char *refname = states.stale.items[i].util;
>  
> -		string_list_insert(&delete_refs_list, refname);
> -
>  		if (!dry_run)
>  			result |= delete_ref(refname, NULL, 0);
>  
> diff --git a/refs.c b/refs.c
> index 5ff457e..2333a9b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
> +int repack_without_refs(struct string_list *without, struct strbuf *err)
>  {
>  	struct ref_dir *packed;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *ref_to_delete;
> -	int i, ret, removed = 0;
> +	int count, ret, removed = 0;
>  
>  	assert(err);
>  
> -	/* Look for a packed ref */
> -	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++;
>  
> -	/* Avoid locking if we have nothing to do */
> -	if (i == n)
> -		return 0; /* no refname exists in packed refs */
> +	/* No refname exists in packed refs */
> +	if (!count)
> +		return 0;
>  
>  	if (lock_packed_refs(0)) {
>  		unable_to_lock_message(git_path("packed-refs"), errno, err);
> @@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  	packed = get_packed_refs(&ref_cache);
>  
>  	/* Remove refnames from the cache */
> -	for (i = 0; i < n; i++)
> -		if (remove_entry(packed, refnames[i]) != -1)
> +	for_each_string_list_item(ref_to_delete, without)
> +		if (remove_entry(packed, ref_to_delete->string) != -1)
>  			removed = 1;
>  	if (!removed) {
>  		/*
> @@ -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;
> +	struct string_list_item *ref_to_delete;
>  
>  	assert(err);
>  
> @@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		return 0;
>  	}
>  
> -	/* Allocate work space */
> -	delnames = xmalloc(sizeof(*delnames) * n);
> -
>  	/* Copy, sort, and reject duplicate refs */
>  	qsort(updates, n, sizeof(*updates), ref_update_compare);
>  	if (ref_update_reject_duplicates(updates, n, err)) {
> @@ -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);
>  		}
>  	}
>  
> -	if (repack_without_refs(delnames, delnum, err)) {
> +	if (repack_without_refs(&refs_to_delete, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;
>  		goto cleanup;
>  	}
> -	for (i = 0; i < delnum; i++)
> -		unlink_or_warn(git_path("logs/%s", delnames[i]));
> +	for_each_string_list_item(ref_to_delete, &refs_to_delete)
> +		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
>  	clear_loose_ref_cache(&ref_cache);
>  
>  cleanup:
> @@ -3833,7 +3832,7 @@ cleanup:
>  	for (i = 0; i < n; i++)
>  		if (updates[i]->lock)
>  			unlock_ref(updates[i]->lock);
> -	free(delnames);
> +	string_list_clear(&refs_to_delete, 0);
>  	return ret;
>  }
>  
> diff --git a/refs.h b/refs.h
> index 2bc3556..0416e5f 100644
> --- 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);
>  
>  extern int ref_exists(const char *);