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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 19, 2014, 20:44 UTC
Message-ID: <20141119204450.GX6527@google.com>
In-Reply-To: <1416423000-4323-1-git-send-email-sbeller@google.com>

Stefan Beller wrote:

> This patch doesn't intend any functional changes.

Yay. :)

> a refactoring, which replaces a char** array by a stringlist
> in the function repack_without_refs.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code and less pointers.

Please wrap to a consistent width and add a blank line between paragraphs. So, either:

	... repack_without_refs.  This is easier to read and ...

or:

	... repack_without_refs.

	This is easier to read and ...

[...]

> +++ b/builtin/remote.c
> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)

[...]

Show 6 quoted lines

> @@ -1325,6 +1320,9 @@ 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_append(&delete_refs, states.stale.items[i].util);

warn_dangling_symref requires a sorted list.  Possible fixes:

 (a) switch to string_list_insert, or
 (b) [nicer] call sort_string_list before the warn_dangling_symrefs
     call.

[...]

Show 28 quoted lines

> --- a/refs.c
> +++ b/refs.c
> @@ -2639,23 +2639,26 @@ 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 ret, needs_repacking = 0, removed = 0;
>  
>  	assert(err);
>  
>  	/* Look for a packed ref */
> -	for (i = 0; i < n; i++)
> -		if (get_packed_ref(refnames[i]))
> +	for_each_string_list_item(ref_to_delete, without) {
> +		if (get_packed_ref(ref_to_delete->string)) {
> +			needs_repacking = 1;
>  			break;
> +		}
> +	}
>  
> -	/* Avoid locking if we have nothing to do */

This comment was helpful --- it's sad to lose it (but if you feel strongly about it then I don't mind).

Show 5 quoted lines

> -	if (i == n)
> -		return 0; /* no refname exists in packed refs */
> +	/* No refname exists in packed refs */
> +	if (!needs_repacking)
> +		return 0;

I kind of liked the 'i == n' test that avoided needing a new auxiliary variable. This is fine and probably a little clearer, though.

[...]

Show 16 quoted lines

> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,14 @@ 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);
> +/*
> + * Repacks the refs pack file excluding the refs given
> + * without: The refs to be excluded from the new refs pack file,
> + *          May be unsorted
> + * err: String buffer, which will be used for reporting errors,
> + *      Must not be NULL
> + */
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);

(nit) Other comments in this file use the imperative mood to describe what a function does, so it would be a little clearer to do that here, too ("Repack the ..." instead of "Repacks the ...").

It might be just me, but I find this formatted comment with everything jammed together hard to read. I'd prefer a simple paragraph, like:

	/*
	 * Remove the refs listed in 'without' from the packed-refs file.
	 * On error, packed-refs will be unchanged, the return value is
	 * nonzero, and a message about the error is written to the 'err'
	 * strbuf.
	 */

Thanks, Jonathan