From: Dan Carpenter <dan.carpenter@oracle.com>
To: Karolina Drobnik <karolinadrobnik@gmail.com>
Cc: outreachy-kernel@googlegroups.com, gregkh@linuxfoundation.org,
forest@alittletooquiet.net, linux-staging@lists.linux.dev,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments
Date: Mon, 8 Nov 2021 13:16:11 +0300 [thread overview]
Message-ID: <20211108101611.GE2026@kadam> (raw)
In-Reply-To: <0a4c26bdabadd666ddf124bbb767e5cc802af937.camel@gmail.com>
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
next prev parent reply other threads:[~2021-11-08 10:16 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-01 14:31 [PATCH v2 0/8] subject: staging: vt6655: Fix line wrapping in `RFvWriteWakeProgSyn` Karolina Drobnik
2021-11-01 14:31 ` [PATCH v2 1/8] staging: vt6655: Introduce `idx` temporary variable Karolina Drobnik
2021-11-04 12:09 ` Dan Carpenter
2021-11-04 14:33 ` Joe Perches
2021-11-05 12:51 ` Dan Carpenter
2021-11-01 14:32 ` [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx` Karolina Drobnik
2021-11-04 12:15 ` Dan Carpenter
2021-11-04 13:00 ` Dan Carpenter
2021-11-04 14:44 ` Joe Perches
2021-11-04 17:40 ` Dan Carpenter
2021-11-01 14:32 ` [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments Karolina Drobnik
2021-11-04 13:38 ` Dan Carpenter
2021-11-08 9:57 ` Karolina Drobnik
2021-11-08 10:16 ` Dan Carpenter [this message]
2021-11-01 14:32 ` [PATCH v2 4/8] staging: vt6655: Rewrite conditional in AL7320 initialization Karolina Drobnik
2021-11-01 14:32 ` [PATCH v2 5/8] staging: vt6655: Rename `RFvWriteWakeProgSyn` function Karolina Drobnik
2021-11-01 14:32 ` [PATCH v2 6/8] staging: vt6655: Update comment for `rf_write_wake_prog_syn` Karolina Drobnik
2021-11-01 14:32 ` [PATCH v2 7/8] staging: vt6655: Delete bogus check for `init_count` in AL2230 Karolina Drobnik
2021-11-01 14:32 ` [PATCH v2 8/8] staging: vt6655: Delete bogus check for `init_count` in AL7230 Karolina Drobnik
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=20211108101611.GE2026@kadam \
--to=dan.carpenter@oracle.com \
--cc=forest@alittletooquiet.net \
--cc=gregkh@linuxfoundation.org \
--cc=karolinadrobnik@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=outreachy-kernel@googlegroups.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;
as well as URLs for NNTP newsgroup(s).