From: Karthik Nayak Date: Sun, 26 Jul 2015 05:40:10 GMT Subject: Re: [PATCH v4 02/10] ref-filter: make the 'color' use ref_formatting_state Message-ID: In-Reply-To: On Sun, Jul 26, 2015 at 9:42 AM, Eric Sunshine wrote: > > 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. > > 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