linux-kernel - Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 8 Nov 2021 13:16:11 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Karolina Drobnik <karolinadrobnik@...il.com>
Cc:     outreachy-kernel@...glegroups.com, gregkh@...uxfoundation.org,
        forest@...ttletooquiet.net, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

On Mon, Nov 08, 2021 at 09:57:25AM +0000, Karolina Drobnik wrote:
> > This commit is cleaning something that was left in a different patch
> > in the same patch set.  Just merge it into the original patch.  Don't
> > make a mess and then fix it.
> 
> I tried adding more than one logical change per patch some time ago and
> Greg asked me to stop doing this.
> 
> > It's tricky to know how to break up patches.  My suggestion is:
> > patch 1: remove all the unnecesary (unsigned short) casts
> > patch 2: merge the rest of patches 1-3 together and send it at once
> 
> Sounds good. If Greg is happy with your approach, I can merge these
> patches, no problem.

The one thing per patch is tricky, but it means one *whole* thing per
patch, not half a thing per patch.  This patch does the anti-pattern
of half do something and then clean up the fall out later.  Sometimes
that is actually a good approach because it can make reviewing easier
if you're doing a ton of similar changes.

The one thing per patch is also about how you present it in the commit
message.  One way of thinking about this is that your first patch
introduces static warnings about "idx" set but not used warnings so you
*must* fix it in the same patch.  At that point it's not an optional
part of the fix.  If it were just a related cleanup then "Oh, well,
that could go either way." but now that you point out the static checker
warning then it's not optional or it sort of almost violates the rule on
bisectable code.

regards,
dan carpenter