Stefan Beller wrote: > From: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> > > 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 [...] > @@ -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). [...] > --- 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@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html