[PATCH 0/6] repack_without_refs(): convert to string_list

[Michael Haggerty <mhagger@alum.mit.edu>]

This is basically an atomized version of Ronnie/Jonathan/Stefan's
patch [1] "refs.c: use a stringlist for repack_without_refs". But I've
actually rewritten most of it from scratch, using the original patch
as a reference.

I was reviewing the original patch and it looked mostly OK [2], but I
found it hard to read because it did several steps at once. So I tried
to make the same basic change, but one baby step at a time. This is
the result.

I'm a known fanatic about making the smallest possible changes in each
commit. The goal is to make the patch series as readable as possible,
because reviewers' time is in shorter supply than coders' time.

* Tiny little patches are IMO usually much easier to read than big
  ones, because there is less to keep in mind at a time.

* Often tiny changes (e.g., renaming variables or functions) are so
  blindingly obvious that one only has to skim them, or even trust
  that the author, with the help of the compiler, could hardly have
  made a mistake [3].

* Using baby steps keeps the author from introducing unnecessary
  changes ("code churn"), by forcing him/her to justify each change on
  its own merits.

* Using baby steps makes it harder for substantive changes to get
  overlooked or to sneak in without discussion [4].

* If there is a problem, baby commits can be bisected, usually making
  it obvious why the bug arose.

* If the mailing list doesn't like part of the series, it is usually
  easier to omit a patch from the next reroll than to extract one
  change out of a patch that contains multiple logical changes.

* It is often possible to arrange the order of the patches to give the
  patch series a good "narrative".

Some members of the community probably disagree with me. Using baby
step patches means that there is more mailing list traffic and more
commits that accumulate in the project's history. There is sometimes a
bit of extra to-and-fro as code is mutated incrementally. Or maybe
other people can just keep more complicated changes in their heads at
one time than I can.

Nevertheless, I submit this version of the patch series for your
amusement. Feel free to ignore it.

[1] http://mid.gmane.org/1416434399-2303-1-git-send-email-sbeller@google.com
[2] Problems that I noticed:

    * The commit message refers to "stringlist" where it should be
      "string_list".

    * One of the loops in prune_remote() iterates using indexes, while
      another loop (over the same string_list) uses
      for_each_string_list_item().

    * The change from using string_list_insert() to string_list_append()
      in the same function, followed by sort_string_list(), doesn't remove
      duplicates as the old version did. The commit message should
      justify that this is OK.

[3] I love the quote from C. A. R. Hoare:

        There are two ways of constructing a software design: One way
        is to make it so simple that there are obviously no
        deficiencies, and the other way is to make it so complicated
        that there are no obvious deficiencies.

    I think the same thing applies to patches.

[4] Case in point: when I was writing the commit message for patch
    3/6, I realized that string_list_insert() omits duplicates whereas
    string_list_append() obviously doesn't. This aspect of the change
    wasn't justified. Do we have to add a call to
    string_list_remove_duplicates()? It turns out that the list cannot
    contain duplicates, but it took some digging to verify this.

Michael Haggerty (6):
  prune_remote(): exit early if there are no stale references
  prune_remote(): initialize both delete_refs lists in a single loop
  prune_remote(): sort delete_refs_list references en masse
  repack_without_refs(): make the refnames argument a string_list
  prune_remote(): rename local variable
  prune_remote(): iterate using for_each_string_list_item()

 builtin/remote.c | 59 ++++++++++++++++++++++++++------------------------------
 refs.c           | 38 +++++++++++++++++++-----------------
 refs.h           | 11 ++++++++++-
 3 files changed, 57 insertions(+), 51 deletions(-)

-- 
2.1.3