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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 20, 2014, 02:15 UTC
Message-ID: <20141120021540.GF6527@google.com>
In-Reply-To: <1416434399-2303-1-git-send-email-sbeller@google.com>

Stefan Beller wrote:

Show 7 quoted lines

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> 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.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code and less pointers.

Thanks for the quick turnaround.

Nit: please wrap to a consistent width and put a blank line between
paragraphs.

That is, the above should either say

	This patch doesn't intend any functional changes.  It is just
	a refactoring to replace a char** array with a string_list
	in the function repack_without_refs.  This is easier to read
	and maintain as it delivers the same functionality with less
	code and fewer pointers.

or

	This patch doesn't intend any functional changes.  It is just
	a refactoring to replace a char** array with a string_list
	in the function repack_without_refs.

	This is easier to read and maintain as it delivers the same
	functionality with less code and fewer pointers.

Although I'm not sure the main benefit is having fewer asterisks. ;-)

[...]

> +++ b/builtin/remote.c

[...]

Show 12 quoted lines

> @@ -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).

[...]

Show 15 quoted lines

> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,15 @@ 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);
> +/*
> + * 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.
> + *
> + * The refs in 'without' may have any order, the err buffer must not be ommited.

Nits:

s/ommited/omitted/

Comma splice.  Long line.

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.

With or without the changes suggested above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>