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/[email protected]
[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
