Re: [PATCH] refs.c: use a stringlist for repack_without_refs
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:02:59
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] refs.c: use a stringlist for repack_without_refs · Stefan Beller <hidden>
- 2016-06-15 · Re: [PATCH] refs.c: use a stringlist for repack_without_refs · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] refs.c: use a stringlist for repack_without_refs · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] refs.c: use a stringlist for repack_without_refs · Jonathan Nieder <hidden>
- 2016-06-15 · Re: [PATCH] refs.c: use a stringlist for repack_without_refs · Stefan Beller <hidden>
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 ... [...]
quoted hunk ↗ jump to hunk
+++ b/builtin/remote.c@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
[...]
quoted hunk ↗ jump to hunk
@@ -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.
[...]quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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