Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:03:00
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs · Junio C Hamano <hidden>
Stefan Beller wrote:
From: Ronnie Sahlberg <redacted> 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. ;-) [...]
quoted hunk ↗ jump to hunk
+++ b/builtin/remote.c
[...]
quoted hunk ↗ jump to hunk
@@ -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). [...]
quoted hunk ↗ jump to hunk
--- 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 <redacted>