Re: [PATCH] refs.c: use a stringlist for repack_without_refs

On Tue, Nov 18, 2014 at 3:45 PM, Jonathan Nieder [off-list ref] wrote:

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.

Yes, I'll do a resend, removing this paragraph.

quoted

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?

Essentially it's simpler to read and maintain as we're having less
lines of code.
I'll add that to the commit message instead.

quoted

Idea-by: Ronnie Sahlberg [off-list ref]
Signed-off-by: Stefan Beller <redacted>

Isn't the patch by Ronnie?

As it was part of the ref-transaction-rename series, it was authored by Ronnie.
Porting it back to the master branch brought up so many conflicts,
that I decided
to rewrite it from scratch while having an occasional look at the
original patch.

If you want we can retain Ronnies authorship, however I may have messed up
the rewriting, so I put my name as author and Ronnie as giving the idea.

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

Sounds reasonable, I can do something similar.

  git log origin/pu --grep='jc:'

for more examples of that.

Some nits below.

Because of the nits, I'd rather be blamed. :)

quoted

--- a/builtin/remote.c
+++ b/builtin/remote.c

[...]

quoted

@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)

[...]

quoted

      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.

will fix

quoted

              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.

quoted

      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.

done

[...]

quoted

+++ b/refs.c

[...]

quoted

@@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)

[...]

quoted

-     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?

no, dropped the dropping in the reroll.

quoted

-     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?

done

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?

[...]

quoted

@@ -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).

ok

[...]

quoted

@@ -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.

ok

[....]

quoted

--- 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?)

ok, I tried adding comments.

Thanks and hope that helps,
Jonathan