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@vger.kernel.org/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 <sahlb...@google.com>
> Signed-off-by: Stefan Beller <sbel...@google.com>
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
