linux-kernel - Re: [GIT PULL] KVM, AMD PSP and ARM CoreSight changes for 5.13 merge window

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]

Message-ID: <CAHk-=wjMc55HFTxEiJi=3iUcFm7fBeYHUvUmP1ZFYxbbs8nfXA@mail.gmail.com>
Date:   Sat, 1 May 2021 10:10:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [GIT PULL] KVM, AMD PSP and ARM CoreSight changes for 5.13 merge window

Ok, got around to this now, one comment:

On Wed, Apr 28, 2021 at 4:05 PM Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> - the coresight/next-ETE-TRBE branch from the KVM ARM tree hasn't yet
> reached you, so I am CCing the maintainer.  Since he sent the patches
> as a pull request to Marc Zyngier (the KVM ARM maintainer) at
> https://lore.kernel.org/kvmarm/20210406224437.330939-1-mathieu.poirier@linaro.org/T/#u,
> I actually suspect that from his point of view he's done.

So the problem with this is not the code, it's the merge (and
admittedly the pull request in that case).

The totality of the merge message for the coresight pull is this:

    Merge remote-tracking branch 'coresight/next-ETE-TRBE' into
kvmarm-master/next

    Signed-off-by: Marc Zyngier <maz@...nel.org>

Can you spot the problem?

And  honestly, it's not just that merge. *Most* of the merges in this
tree have absolutely garbage commit messages. This is particularly
true of Marc's merges, but there's one from you too, with the merge
message being:

    Merge branch 'kvm-sev-cgroup' into HEAD

Guys, merges need explanations. A one-liner "I merged this" is not ok.
The reason I ask for pull requests to have explanations is exactly so
that I can write reasonable merge messages.

Pull requests need to have explanations of what they pull - not just
because it needs to go into the merge message, but because the
maintainer needs to keep track of what's happening.

And even when you merge your own topic branch, you should explain
*what* you are merging and why.

Yes, it can be some simple extra line for trivial stuff ("Fix ARM
memory slot handling"), but even when it's that simple, that extra
line should be in addition to the "this is where I merged things from"
like

    Merge branch 'kvm-arm64/memslot-fixes' into kvmarm-master/next

so sometimes you only need one extra short line as a human-readable
"this is what's going on".

But then when you have something like that commit 53648ed3f085

    Merge remote-tracking branch 'coresight/next-ETE-TRBE' into
kvmarm-master/next

that actually brings in a lot of new code, that "merge from where"
really doesn't cut it.

Put another way - just look at

    gitk 53648ed3f085^..53648ed3f085^2

and tell me that that merge message is enough for what got merged.

Because it damn well isn't.

Merges are *important*. Even if nothing goes wrong, that's where
history can get messy, and the message really tells outsides what's
going on in the big picture.

And heaven forbid that a bisect points to a merge as an issue - it's
rare, but it happens - then you really want the merge to talk about
what is going on, and what it brings in.

So please people: fix your useless merge messages.

            Linus