Re: [PATCH v2 23/33] diff-merges: fix style of functions definitions
- From
Sergey Organov <sorganov@gmail.com>
- Date
- Dec 18, 2020, 21:23 UTC
- Message-ID
- <871rfm95b9.fsf@osv.gnss.ru>
- In-Reply-To
- <CABPp-BGPZiwjSzZw5PLwkctW7hnG2S6UGHmkTGCh1BqgXJ+vEQ@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 56 quoted lines
> On Fri, Dec 18, 2020 at 5:41 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> Put open curly brace on its own line
>> >>
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >> diff-merges.c | 36 ++++++++++++++++++++++++------------
>> >> 1 file changed, 24 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/diff-merges.c b/diff-merges.c
>> >> index cba391604ac7..0165fa22fcd1 100644
>> >> --- a/diff-merges.c
>> >> +++ b/diff-merges.c
>> >> @@ -2,7 +2,8 @@
>> >>
>>
>> [...]
>>
>> >>
>> >> -void diff_merges_set_dense_combined_if_unset(struct rev_info *revs) {
>> >> +void diff_merges_set_dense_combined_if_unset(struct rev_info *revs)
>> >> +{
>> >> if (!revs->combine_merges)
>> >> set_dense_combined(revs);
>> >> }
>> >> --
>> >> 2.25.1
>> >>
>> >
>> > But...didn't you add all these functions yourself earlier in the
>> > series?
>>
>> Yes, I did indeed and somehow picked wrong style from one of the
>> occurrences of this style in the existing Git codebase.
>>
>> > Why didn't you split this patch up and squash it into the
>> > relevant previous patches?
>>
>> When Junio noticed and pointed to this deficiency, I asked him if I
>> should fix all the series from the start, or it'd be OK to use fixup
>> commit. As he didn't answer and nobody else commented either, I opted
>> for the latter.
>>
>> I can still do it if it's that essential, but I'd prefer not to, to
>> avoid both the hand-work and causing entire series to change. The
>> problem is that there were code movements in the series, so such a fix
>> to earlier patches would cause conflicts down the commits chain, to be
>> resolved by hand.
>
> That's what add -p and interactive rebase is for. :-) Code is read
> more than it is written, so it's important to get things clean. And
> not just for immediate reviewers, but for people who look at it later.I do know how to do it, but I'd still prefer it to be accepted in its current form, as I don't see this particular case being that important to justify rewriting of all the series. I'll go through the pain if it's a show-stopper though.
Thanks, -- Sergey