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

From: Stefan Beller <sbeller@google.com>
Date: Nov 21, 2014, 19:57 UTC
Message-ID: <CAGZ79kaGuMNO7_ynRMO_8T2shRn=S-gctos6WJL=gMOsDitM+w@mail.gmail.com>
In-Reply-To: <xmqq61e81ljq.fsf@gitster.dls.corp.google.com>

On Fri, Nov 21, 2014 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I don't think that those iterations changed anything substantial that
>> overlaps with my version, but TBH it's such a pain in the ass working
>> with patches in email that I don't think I'll go to the effort of
>> checking for sure unless somebody shows interest in actually using my
>> version.
>>
>> Sorry for being grumpy today :-(

Sorry for causing the grumpyness. I have compared the versions, and they do look pretty similar. In refs.{c,h} we're just talking about variable names and comments, that are different.

In remote.c prune_remote however we did have slight differences,
* early exit vs a large body below an if
* your approach seems more elegant to me as you seem to know what you're doing:
       for_each_string_list_item(item, &states.stale)
               string_list_append(&refs_to_prune, item->util);
 instead of
       for (i = 0; i < states.stale.nr; i++)
               string_list_append(&delete_refs, states.stale.items[i].util);
* You do not have a sort_string_list at the end before warn_dangling_symrefs,
   but you explained that it is not necessary.

On my continued journey on this mailing list I'll try to follow your example and write lots of small easy to review patches, as they are indeed way easier to follow.

However as Junio mentioned, we get other problems having too small changes. In the review for the [PATCH v3 00/14] ref-transactions-reflog series you said:

> I was reviewing this patch series (I left some comments in Gerrit about
> the first few patches) when I realized that I'm having trouble
> understanding the big picture of where you want to go with this.

Maybe that was just my fault, not having stated the intentions in the cover letter explicit enough. But having many patches will also not help on presenting the big picture easily.

Thanks for bearing with me, Stefan

Show 20 quoted lines

>
> Is the above meant as a grumpy rant to be ignored, or as a
> discussion starter to improve the colaboration to allow people to
> work better together instead of stepping on each other's patches?
>
> FWIW, I liked your rationale for "many smaller steps".
>
> One small uncomfort in that approach is that it often is not very
> obvious by reading "log -p master.." alone how well the end result
> fits together.  Each individual step may make sense, or at least it
> may not make it any worse than the original, but until you apply the
> whole series and read "diff master..." in a sitting, it is somewhat
> hard to tell where you are going.  But this is not "risk" or "bad
> thing"; just something that may make readers feel uncomfortable---we
> are not losing anything by splitting a series into small logical
> chunks.
>
> Thanks.
>
>