Re: [PATCH v4 02/10] ref-filter: make the 'color' use ref_formatting_state

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Jul 25, 2015 at 12:15 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sat, Jul 25, 2015 at 3:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> Make color which was considered as an atom, to use
>>> ref_formatting_state and act as a pseudo atom. This allows
>>> interchangeability between 'align' and 'color'.
>>>
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>> I think 1/10 and 2/10 are the other way around.  Preferably, these
>> would be three patches, in this order:
>>
>>  (1) prepare the "formatting state" and pass it around; no other
>>      code change.
>>
>>  (2) have "color" use that facility.
>>
>>  (3) add a new feature using that facility.
>>
>> The first two may want to be combined into a single patch, if it
>> does not make the patch too noisy.
>
> Ill reverse the patches and merge the first two as you suggested.

While reviewing patch 1/10, it required more effort and was
distracting to have to separate out (mentally) changes which were
specific to the new %(align:X) pseudo-atom and those which introduced
the "formatting state". As such, it probably would be a good idea to
aim for the three distinct patches suggested by Junio rather than
aiming, up front, to merge the first two. After all, they are
conceptually distinct changes, and keeping them separate is friendlier
to reviewers. In the end, you may find that patch 1 is so trivial that
it can be merged with patch 2 without making review more difficult,
however, keeping them distinct during development helps you avoid
conflating unrelated changes.