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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 26, 2015, 05:40 UTC
Message-ID: <CAOLa=ZRz9LL5heW2FOhZje+=5W_Ybtm+Rcsm-RH=eKXoh5yw2Q@mail.gmail.com>
In-Reply-To: <CAPig+cQEKrjePskjSacrf3SbLZR7_ADxTLwcV7vBPcjUxBLiEg@mail.gmail.com>

On Sun, Jul 26, 2015 at 9:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 5 quoted lines

>
> Should 'color' should be declared 'char *' rather than 'const char *'?
> It's always assigned via xstrdup(), and if declared 'char *', you
> wouldn't have to cast away the 'const' when freeing it.
>

yes, will change.

Show 13 quoted lines

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

Yes! I'm doing that like Junio suggested. Thanks for bringing it up :)

-- 
Regards,
Karthik Nayak