Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Ard,

I agree we do not want to code to be worse.  Which I interpret
in this context as introducing extra commits that reduce the
readability of the commits.

We also need to consider the ease of review of patches and clear
responsibility for providing reviews.  Especially when we have
different reviewers/maintainers for different packages. It makes
it easier to review a patch when patches do not cross package
boundaries.  Otherwise, what does a Reviewed-by mean if you
only have context for a portion of the patch?  Does that mean
that reviewer is confident in all the changes including those
they may not have any experience with?

That means there is some tension between readability of commits
and code review of commits.

The other topic brought up in this discussion is bisect.

I understand the value of bisect to help identify the source
of a bug or behavior change.

However, the current EDK II CI system does not test at the
granularity of single commits.  Instead, it tests the entire
patch series in a PR.

I have also observed some maintainers combining multiple
patch series into a single PR.

I also suspect that there are many patch series in the edk2
commit history that would not work if bisect was run across
them today. 

What this means in practice with the current edk2 repository
history is that we do not know if every commit can be built
and run. We only know that the EDK II CI tests that are run
against PRs have passed.

We do expect build/run at the granularity of a PR merge today.
However, there is no information in the git history to know
where the PR merges occur.  If that extra bit of information
was available, then bisect could select the commits at PR 
merge boundaries to look for bug/behavior change.  The history
of merged PRs is in github using the following query.

https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged

I imagine the set of sha hashes at PR boundaries could be extracted
from this query and bisect could select from that set of sha hashes
that are a subset of the total set of sha hashes.  Biset would
identify the specific PR merge where the bug or behavior change was
introduced.

Given the current state of the edk2 repo history, the concern about
splitting up commits on package boundaries that may cause a bisect
failure at a commit boundary within a single patch series may not be
something that needs to be considered.

A refinement to the proposal is to require patches to not cross 
package boundaries and authors simply need to split into multiple
commits based on package boundaries without considering bisect within
a single patch series. That prevents adding extra patches that make
the changes hard to understand and has the additional benefit of 
making the patch series easier to review and clear ownership for
reviewing each patch.

Best regards,

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, February 14, 2024 3:48 PM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Rebecca Cran
> <rebecca@bsdio.com>; Liming Gao <gaoliming@byosoft.com.cn>; Feng, Bob C
> <bob.c.feng@intel.com>; Chen, Christine <yuwei.chen@intel.com>; Michael
> Kubacki <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck:
> Error if commit modifies multiple packages
> 
> On Thu, 15 Feb 2024 at 00:27, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > Using commit message does provide granularity down to a single
> commit,
> > which may be better than PR label which applies across all commits
> > in a series.
> >
> > However, a flag in the commit message can be set by the author who
> may
> > not be a maintainer and maintainers would then need to review commit
> > messages for incorrect use of those flags.
> >
> > Only maintainers/admins can set labels, so from a permission
> management
> > perspective the PR label has an advantage.
> >
> > Additional comments below.  There are ways to support bisect and meet
> > the proposed checks. The suggestion uses techniques that Laszlo
> helped
> > me with in the past when working on issues like there. I have seen
> more
> > complex scenarios than the examples listed below, and have been able
> to
> > figure out a path through.
> >
> > I would agree it is extra work to think about these when working on
> > the code changes and extra work to reformulate the patches when these
> > conditions are encountered.
> >
> > I just want to be clear on the objections.  It is not about if the
> patches
> > can be organized to follow this proposal.  The objection is about the
> > extra work required to reformulate patches.
> >
> 
> No.
> 
> The objection is fundamentally about having to appease the CI even if
> doing so is unreasonable. I don't mind a bit of extra work. I do mind
> having to make code changes that make the code worse just to tick a CI
> box. (This is why I disabled uncrustify for the ARM packages: many
> header files which were perfectly legible before were converted into a
> jumble of alphabet soup because uncrustify removed all of the
> indentation)
> 
> It is about having the discretion to deviate from the rules in the odd
> case where the cure is worse than the disease.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115475): https://edk2.groups.io/g/devel/message/115475
Mute This Topic: https://groups.io/mt/104345509/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-