Re: [PATCH v2 23/33] diff-merges: fix style of functions definitions

[Elijah Newren <newren@gmail.com>]

On Fri, Dec 18, 2020 at 1:23 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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.

Personally, I think that a really important point to keep in mind when
submitting patch series is trying to figure out the easiest way to
move the code from point A to point B, not the route you took to get
from point A to point B.  This is especially true for longer patch
series.  It's common after you've finished a series to discover there
was an easier or cleaner route to follow that would have arrived at
the same end-point.  It's not uncommon for me to spend a significant
chunk of time rebasing and restructuring a patch series to try to
highlight such a better path.  This includes not just style fixups,
but different patch orderings, alternate ways to break up functions,
using different data structures, etc.

All that said, I don't get to choose what's accepted or not, so I
can't say if it'll be required.  What I can say is I like your series
and I think it provides obvious improvements, but I'd still recommend
against its inclusion as it stands for as long as this patch is still
present.  (I have a feeling that there are other things in this series
that possibly should be squashed, restructured, or refactored, e.g.
[1]).  This is just my opinion, of course.  You are free to disagree
with me, and wait to see what others say and what Junio does.


[1] https://lore.kernel.org/git/CABPp-BESWpqska++EsfxfbncyV0kNo1RGLjF+1BiV=D6zLx2LQ@mail.gmail.com/