Re: [PATCH v2 23/33] diff-merges: fix style of functions definitions
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 18, 2020, 16:12 UTC
- Message-ID
- <CABPp-BGPZiwjSzZw5PLwkctW7hnG2S6UGHmkTGCh1BqgXJ+vEQ@mail.gmail.com>
- In-Reply-To
- <87blercju2.fsf@osv.gnss.ru>
On Fri, Dec 18, 2020 at 5:41 AM Sergey Organov <sorganov@gmail.com> wrote:
Show 51 quoted lines
>
> 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.