Stefan Beller wrote: > This patch was heavily inspired by a part of the ref-transactions-rename > series[1], but people tend to dislike large series and this part is > relatively easy to take out and unrelated, so I'll send it as a single > patch. > > [1] https://www.mail-archive.com/git@xxxxxxxxxxxxxxx/msg60604.html The above is a useful kind of comment to put below the three-dashes. It doesn't explain what the intent behind the patch is, why I should want this patch when considering whether to upgrade git, or what is going to break when I consider reverting it as part of fixing something else, so it doesn't belong in the commit message. > 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. Thanks. Why, though? Is it about having something simpler to pass from builtin/remote.c::remove_branches(), or something else? > Idea-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> > Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> Isn't the patch by Ronnie? Sometimes I send a patch by someone else and make some change that I don't want them to be blamed for. Then I keep their sign-off and put a note in the commit message about the change I made. See output from git log origin/pu --grep='jc:' for more examples of that. Some nits below. > --- a/builtin/remote.c > +++ b/builtin/remote.c [...] > @@ -1325,6 +1319,11 @@ 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_insert(&delete_refs_list, > + states.stale.items[i].util); > + > + > if (states.stale.nr) { (style) The double blank line looks odd here. > printf_ln(_("Pruning %s"), remote); > printf_ln(_("URL: %s"), > @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run) > ? states.remote->url[0] > : _("(no URL)")); > > - delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs)); Now that there's no delete_refs array duplicating the string list, would it make sense to rename delete_refs_list to delete_refs? As a nice side-effect, that would make the definition of delete_refs_list and other places it is used appear in the patch. > for (i = 0; i < states.stale.nr; i++) { > const char *refname = states.stale.items[i].util; (optional) this could be for_each_string_list_item(ref, &delete_refs_list) { const char *refname = ref->string; ... which saves the reader from having to remember what states.stale.items means. [...] > +++ b/refs.c [...] > @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err) [...] > - int i, ret, removed = 0; > + int count, ret, removed = 0; > > assert(err); > > - /* Look for a packed ref */ The old code has comments marking sections of the function: /* Look for a packed ref */ /* Avoid processing if we have nothing to do */ /* Remove refnames from the cache */ /* Remove any other accumulated cruft */ /* Write what remains */ Is dropping this comment intended? > - for (i = 0; i < n; i++) > - if (get_packed_ref(refnames[i])) > - break; > + count = 0; > + for_each_string_list_item(ref_to_delete, without) > + if (get_packed_ref(ref_to_delete->string)) > + count++; The old code breaks out early as soon as it finds a ref to delete. Can we do similar? E.g. for (i = 0; i < without->nr; i++) if (get_packed_ref(without->items[i].string)) break; (not about this patch) Is refs_to_delete leaked? [...] > @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, > int ref_transaction_commit(struct ref_transaction *transaction, > struct strbuf *err) > { > - int ret = 0, delnum = 0, i; > - const char **delnames; > + int ret = 0, i; > int n = transaction->nr; > struct ref_update **updates = transaction->updates; > + struct string_list refs_to_delete = STRING_LIST_INIT_DUP; The old code doesn't xstrdup the list items, so _NODUP should work fine (and be slightly more efficient). [...] > @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction, > } > > if (!(update->flags & REF_ISPRUNING)) > - delnames[delnum++] = update->lock->ref_name; > + string_list_insert(&refs_to_delete, > + update->lock->ref_name); string_list_append would be analagous to the old code. [....] > --- a/refs.h > +++ b/refs.h > @@ -163,8 +163,7 @@ 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); > +extern int repack_without_refs(struct string_list *without, struct strbuf *err); A comment could mention whether the ref list needs to be sorted. (It doesn't, right?) Thanks and hope that helps, Jonathan -- 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