public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: "Theodore Ts'o" <tytso@mit.edu>
To: Liu Peibao <liupeibao@163.com>
Cc: adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'
Date: Sun, 19 Jun 2022 14:18:22 -0400	[thread overview]
Message-ID: <Yq9obvFIv8LjAAvg@mit.edu> (raw)
In-Reply-To: <c19b8c8f-7c0f-33e6-3f2c-3425dee7fa8d@163.com>

On Sun, Jun 19, 2022 at 11:21:27AM +0800, Liu Peibao wrote:
> 
> Thanks for your reply. What I want do to is rename some temporary variables
> in the patch2 and when I make the patch, there are the checkpatch warnings.
> From the point of view "one patch do one thing", I split the modification
> into two patches. Thanks!

I didn't really see the poiont of renaming the temporary variables,
either.

In this particular case basically only used to avoid line lengths from
exceeding ~72 characters, and requiring a line wrap, and bio_start and
bio_end is used only in one place in the code block below.

Is it _really_ all that confusing whether they are named
bio_{start,end} instead of bvec_{start,end}?

If I was writing that code from scratch, I might have just used start
and end without any prefixes.  And as far as "only have a patch do one
thing at a time", this doesn't apply to checkpatch fixes.

The basic motivation behind "no checkpatch-only fixes" is that it
tends to introduce code churn which makes interpreting information
from "git blame" more difficult; and so therefore the costs exceed the
extremely marginal benefits of fixing most checkpatch complaints.  So
making a _patch_ be checkpatch clean, whether it's modifying existing
code or writing new code, is fine, since you're making a subtantive
change to the code, so this is as good a time as any to fix up tiny
nits such as checkpatch complaints.

But the idea behind "no unnecessary code churn since it ruins git
blame and could potentially induce future patch conflicts" also
applies to renaming variables.  The benefits are very minor, and they
don't outweigh the costs.

						- Ted

  reply	other threads:[~2022-06-19 18:18 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-18 12:01 [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned' Liu Peibao
2022-05-18 12:01 ` [PATCH 2/2] ext4: rename temporary variables in ext4_finish_bio() Liu Peibao
2022-06-16 14:49 ` [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned' Theodore Ts'o
2022-06-19  3:21   ` Liu Peibao
2022-06-19 18:18     ` Theodore Ts'o [this message]
2022-06-21 14:28       ` Liu Peibao

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=Yq9obvFIv8LjAAvg@mit.edu \
    --to=tytso@mit.edu \
    --cc=adilger.kernel@dilger.ca \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liupeibao@163.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox