linux-kernel - Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Feb 2020 20:48:03 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Joe Perches <joe@...ches.com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Andy Whitcroft <apw@...onical.com>
Subject: Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after
 first Signed-off-by line

On Mon, Feb 24, 2020 at 6:13 PM Joe Perches <joe@...ches.com> wrote:
>
> On Mon, 2020-02-24 at 23:58 +0000, John Stultz wrote:
> > Quite often, the Change-Id may be between Signed-off-by: lines or
> > at the end of them. Unfortunately checkpatch won't catch these
> > cases as it disables in_commit_log when it catches the first
> > Signed-off-by line.
> >
> > This has bitten me many many times.
>
> Hmm.  When is change-id used in your workflow?

Since I have a few kernel repos that I use for both upstream work and
work targeting AOSP trees, I usually have the gerrit commit hook
enabled in my tree (its easier to strip with sed then it is to re-add
after submitting to gerrit), and at least the commit-msg hook I have
will usually append a Change-Id: line at the end of the commit
message, usually after the signed-off-by line.

Even in the example in the README from:
  https://android.googlesource.com/kernel/common/+/android-mainline
shows how one might have the change-id and other AOSP tags added after
the existing sob-chain. So it doesn't seem to be that rare.

Some other examples from the android-mainline tree:
https://android.googlesource.com/kernel/common/+/5fba1b18cfc72e264e5f3ce49020ed322aa6ac9f
https://android.googlesource.com/kernel/common/+/6ea0a439a15ba42b6c5f81618e53d5c61f89e4ac
https://android.googlesource.com/kernel/common/+/99f4553ab4a6b008b37e878f7046a2202cdb2ec4

> > I suspect this patch will break other use cases, so it probably
> > shouldn't be merged, but I wanted to share it just to help
> > illustrate the problem.
> >
> > Cc: Andy Whitcroft <apw@...onical.com>
> > Cc: Joe Perches <joe@...ches.com>
> > Signed-off-by: John Stultz <john.stultz@...aro.org>
>
> Yes, I expect this will break things.

Suggestions for a better approach? I can't say I'm very familiar with
checkpatch's code.

> And it's probably better to not add a Signed-off-by: when
> you intend this not to be merged.

So, I try to add Sign-off-by to all the patches I send as it certifies
that I wrote it or otherwise have the right to pass it on as an
open-source patch - not as "ok to merge" criteria.

More vendor code then I'd like is usually not intended to be merged
upstream, but it's still important that folks sign off their patches.
:)

thanks
-john