Date: Mon, 24 Oct 2016 19:46:23 +0100
From: Andy Whitcroft <apw@canonical.com>
To: Joe Perches <joe@perches.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] checkpatch: remove false warning for commit reference
Message-ID: <20161024184623.rfhlx337xvynydcr@brain>
References: <1465608362-4677-1-git-send-email-xypron.glpk@gmx.de>
 <1477208091-4887-1-git-send-email-xypron.glpk@gmx.de>
 <1477255021.3561.9.camel@perches.com>
 <ae7cd959-d5b5-d76f-1926-16bcecbaa916@gmx.de>
 <1477334385.1984.9.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1477334385.1984.9.camel@perches.com>
User-Agent: Mutt/1.6.2-neo (2016-08-21)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2829
Lines: 75

On Mon, Oct 24, 2016 at 11:39:45AM -0700, Joe Perches wrote:
> On Mon, 2016-10-24 at 19:22 +0200, Heinrich Schuchardt wrote:
> > On 10/23/2016 10:37 PM, Joe Perches wrote:
> > > On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote:
> > > > Checkpatch warns of an incorrect commit reference style
> > > > for any hexadecimal number of 12 digits and more.
> > > > 
> > > > Numbers of 12 digits are not necessarily commit ids.
> > > > 
> > > > For an example provoking the problem see
> > > > https://patchwork.kernel.org/patch/9170897/
> > > > 
> > > > Checkpatch should only warn if the number refers to an
> > > > existing commit.
> > > 
> > > That seems sensible.
> > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >  scripts/checkpatch.pl | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > 
> > > []
> > > > @@ -848,6 +848,7 @@ sub git_commit_info {
> > > >  #		    echo "commit $(cut -c 1-12,41-)"
> > > >  #		done
> > > >  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
> > > > +		$id = undef;
> > > 
> > > OK
> > > 
> > > >  	} else {
> > > >  		$id = substr($lines[0], 0, 12);
> > > >  		$desc = substr($lines[0], 41);
> > > > @@ -2577,7 +2578,9 @@ sub process {
> > > >  			($id, $description) = git_commit_info($orig_commit,
> > > >  							      $id, $orig_desc);
> > > >  
> > > > -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> > > > +			if ($id && ($short || $long || $space || $case
> > > > +				|| ($orig_desc ne $description)
> > > > +				|| !$hasparens)) {
> > > 
> > > I'd prefer
> > > 			if (defined($id) &&
> > 
> > For $id = 0 or $id = "" this would make a difference.
> > Surely I can update the patch like this to make the test more readable.
> > 
> > > 			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> > > 
> > > and not wrap the tests.
> > 
> > Putting defined($id) on a line of its own is fine.
> > 
> > Not wrapping the tests will produce a line of over 80 characters and
> > give a warning in scripts/checkpatch.pl.
> > 
> > The script should respect the standards it imposes on others.
> 
> Nope.
> 
> I never bother using checkpatch on checkpatch.
> perl is unreadable enough with having to wrap long and
> complex regexes on 80 columns.

More specifically checkpatch is primarily checking C language construction
to keep the style of the Linux kernel consistent.  The rules were never
really intended to apply to perl or other languages.  We have long leant
towards longer lines in checkpatch for some kind of readability, as much
as you can have in "systactic sugar overload" language.

-apw