All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ben Peart <peartben@gmail.com>
To: Brandon Williams <bmwill@google.com>, Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Stefan Beller <sbeller@google.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [RFC] clang-format: outline the git project's coding style
Date: Fri, 11 Aug 2017 16:06:54 -0400	[thread overview]
Message-ID: <0378b54f-7332-7a39-23b1-40886c97c596@gmail.com> (raw)
In-Reply-To: <20170810213013.GD73298@google.com>



On 8/10/2017 5:30 PM, Brandon Williams wrote:
> On 08/10, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>>> On 08/10, Junio C Hamano wrote:
>>>
>>>> I vaguely recall that there was a discussion to have SubmitGit wait
>>>> for success from Travis CI; if that is already in place, then I can
>>>> sort of see how it would help individual contributors to have the
>>>> style checker in that pipeline as well.
>>>>
>>>> I have a mixed feelings about "fixing" styles automatically, though.
>>>
>>> I still think we are far away from a world where we can fix style
>>> automatically.  If we do want to keep pursuing this there are a number
>>> steps we'd want to take first.
>>>
>>> 1. Settle on a concrete style and document it using a formatter's rules
>>>     (in say a .clang-format file).  This style would most likely need to
>>>     be tuned a little bit, at least the 'Penalty' configuration would
>>>     need to be tuned which (as far as I understand it) is used to
>>>     determine which rule to break first to ensure a line isn't too long.
>>
>> Yes.  I think this is what you started to get the ball rolling.
>> Together with what checkpatch.pl already diagnoses, I think we can
>> get a guideline that is more or less reasonable.
>>
>>> 2. Start getting contributors to use the tool to format their patches.
>>>     This would include having some script or hook that a contributor
>>>     could run to only format the sections of code that they touched.
>>
>> This, too.  Running checkpatch.pl (possibly combined with a bit of
>> tweaking it to match our needs) already catches many of the issues,
>> so a tool with a similar interface would be easy to use, I would
>> imagine.
>>
>>> 3. Slowly the code base would begin to have a uniform style.  At
>>>     some point we may want to then reformat the remaining sections of the
>>>     code base.  At this point we could have some automated bot that fixes
>>>     style.
>>
>> I suspect I am discussing this based on a different assumption.
>>
>> I think the primary goal of this effort is to make it easier to
>> cleanse the new patches that appear on the list of trivial style
>> issues, so that contributors and reviewers do not have to spend
>> bandwidth and brain cycles during the review.  And I have been
>> assuming that we can do so even without waiting for a "tree wide"
>> code churn on existing code to complete.
> 
> Yes that's one of the steps I missed we can call it 2.5 ;)  (3) could be
> a long term goal which is what I was trying to get at by saying:
> 
>>> 3. Slowly the code base would begin to have a uniform style.
> 

Just adding my "Yes!" vote.

Consistent formatting makes the code easier to read and maintain.  I've 
been in religious wars debating whether the opening brace should be on 
the same line or the next line and while I have my personal preferences, 
I can work with just about anything as long as it is consistent. When it 
comes to dealing with a tool, I am willing to live with some "I would 
have wrapped that differently" if I can stop spending so much time 
manually wrapping code.

I think the goals should be to 1) increase readability of the code 2) 
reduce the time spent by reviewers and 3) reduce the time spent by 
contributors.  A pre-commit hook that checked for errors (and gave the 
instructions on how to automatically have the tool correct them) would 
be very helpful.

Please continue to push this forward!

  reply	other threads:[~2017-08-11 20:07 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-08  1:25 [RFC] clang-format: outline the git project's coding style Brandon Williams
2017-08-08 12:05 ` Johannes Schindelin
2017-08-08 17:40   ` Stefan Beller
2017-08-08 18:23     ` Brandon Williams
2017-08-09 22:33       ` Johannes Schindelin
2017-08-09 22:42         ` Stefan Beller
2017-08-10  9:38           ` Johannes Schindelin
2017-08-10 16:41             ` Junio C Hamano
2017-08-10 17:15               ` Brandon Williams
2017-08-10 17:28                 ` Junio C Hamano
2017-08-10 21:30                   ` Brandon Williams
2017-08-11 20:06                     ` Ben Peart [this message]
2017-08-14 15:52               ` Johannes Schindelin
2017-09-28 11:41         ` Johannes Schindelin
2017-09-28 17:16           ` Brandon Williams
2017-08-08 17:45 ` Junio C Hamano
2017-08-08 18:03   ` Brandon Williams
2017-08-08 18:25     ` Junio C Hamano
2017-08-09  7:05       ` Junio C Hamano
2017-08-11 17:49         ` Brandon Williams
2017-08-11 19:00           ` Junio C Hamano
2017-08-09 13:01 ` Jeff King
2017-08-09 14:03   ` Ævar Arnfjörð Bjarmason
2017-08-09 22:53     ` Stefan Beller
2017-08-09 23:11       ` Stefan Beller
2017-08-11 17:52         ` Brandon Williams
2017-08-11 21:18           ` Jeff King
2017-08-12  4:39             ` Junio C Hamano
2017-08-13  4:41               ` Jeff King
2017-08-13 16:14                 ` Ramsay Jones
2017-08-13 16:36                   ` Ramsay Jones
2017-08-13 17:33                   ` Junio C Hamano
2017-08-13 19:17                     ` Ramsay Jones
2017-08-09 23:19       ` Jeff King
2017-08-15  0:40         ` brian m. carlson
2017-08-15  1:03           ` Jonathan Nieder
2017-08-14 21:30 ` [PATCH v2 0/2] clang-format Brandon Williams
2017-08-14 21:30   ` [PATCH v2 1/2] clang-format: outline the git project's coding style Brandon Williams
2017-08-14 22:02     ` Stefan Beller
2017-08-15 13:56       ` Ben Peart
2017-08-15 17:37         ` Brandon Williams
2017-08-14 22:48     ` Jeff King
2017-08-14 22:51       ` Jeff King
2017-08-14 22:54         ` Brandon Williams
2017-08-14 23:01           ` Jeff King
2017-08-16 12:18             ` Johannes Schindelin
2017-08-15 14:28     ` Ben Peart
2017-08-15 17:34       ` Brandon Williams
2017-08-15 17:41         ` Stefan Beller
2017-08-14 21:30   ` [PATCH v2 2/2] Makefile: add style build rule Brandon Williams
2017-08-14 21:53     ` Stefan Beller
2017-08-14 22:25     ` Junio C Hamano
2017-08-14 22:28       ` Stefan Beller
2017-08-14 22:57         ` Jeff King
2017-08-14 23:29           ` Junio C Hamano
2017-08-14 23:47             ` Junio C Hamano
2017-08-15  0:05               ` Brandon Williams
2017-08-15  1:52               ` Jeff King
2017-08-14 21:32   ` [PATCH v2 0/2] clang-format Brandon Williams
2017-08-14 23:06   ` Jeff King
2017-08-14 23:15     ` Stefan Beller
2017-08-15  1:47       ` Jeff King
2017-08-15  3:03         ` Junio C Hamano
2017-08-15  3:38           ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0378b54f-7332-7a39-23b1-40886c97c596@gmail.com \
    --to=peartben@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.