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