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
