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)
[...]
> @@ -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.
[...]
> --- 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).
> - 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.
[...]
> --- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
