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