[EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process
Bret Barkelew <bret.barkelew@...>
“But we do have tools that insert the Bugzilla number in all the commits of the series, assist with the squash, etc.”
Are these internal-only, or are they something we could evaluate when we move to the PR process? If not, are they based on anything we could leverage?
I believe that the plan is to stick with Bugzilla for the immediate future rather than use GitHub Issues (which is probably for the best for now, given the span across repos and access levels), so any tooling to tie that together would be interesting to evaluate.
In Mu we have a similar problem of keeping track of what features/bugs have already been upstreamed and when can they be dropped during an upstream integration, so that’s the more personal interest I have in such automation.
From: Andrew Fish<mailto:afish@...>
Sent: Thursday, May 21, 2020 8:00 PM
To: Laszlo Ersek<mailto:lersek@...>
Cc: firstname.lastname@example.org<mailto:email@example.com>; spbrogan@...<mailto:spbrogan@...>; firstname.lastname@example.org<mailto:email@example.com>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@...>; Bret Barkelew<mailto:Bret.Barkelew@...>; Kinney, Michael D<mailto:michael.d.kinney@...>; Leif Lindholm (Nuvia address)<mailto:leif@...>
Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process
On May 21, 2020, at 6:30 AM, Laszlo Ersek <lersek@...> wrote:In my work world we require code review by a manager and that is the de facto enforcement mechanism. Basically there is always an owner to make sure the process was followed :)
Also in our world the squash is a developer choice. But we do have tools that insert the Bugzilla number in all the commits of the series, assist with the squash, etc.
The question is not how nice it is to use squash-on-merge in theI'd also point out that the processes you chose kind of defines your quanta of work. It is likely you would be willing to tackle a really big change as a large patch set, that you would likely break up into multiple PRs in a squash on commit world. In a squash on commit world you also might break a Bugzilla (BZ) up into dependent BZs, a tree of BZs. That might sound crazy, but when you work on a bigger project and there are BZs for EFI, T2, macOS, the Installer, and the RecoveryOS for a customer visible feature this tree of BZ might be familiar and make sense to you.
But I think the real argument for consistency is we have a rich git history that has value. We have made resource tradeoffs to have that rich git history so to me it makes the most sense, for these project, to try to preserve our past investment in git history.
On 05/22/20 07:48, Bret Barkelew wrote:
In Mu we have a similar problem of keeping track of what features/bugsProposal:
- Whenever upstreaming a bugfix or a feature, open an upstream BZ.
- In your downstream ticket for the same bugfix or feature,
cross-reference the upstream BZ URL. This shouldn't be a normal comment,
but a dedicated field. In Bugzilla, there is "See Also" (it can carry a
list of URLs). In our own (RH) Bugzilla instance, "See Also" has been
replaced with an "External Trackers" list, but the idea is the same.
- When you rebase, run a git-log over the upstream commit history being
straddled, and collect the upstream BZs referenced. For example:
$ git log edk2-stable201911..edk2-stable202002 \
| grep -E -o 'https://bugzilla.tianocore.org/show_bug.cgi\?id=[0-9]+' \
| sort -u
This reliably presents the set of upstream BZs that were *touched on* in
the subject development cycle, because TianoCore contributors diligently
reference BZs in commit messages. Right? :)
- Use a script to fetch the fresh status of each of those BZ URLs,
because in some cases, "touched on a BZ" does not guarantee "fixed BZ".
Some BZs may require multiple waves of patches.
Of course, BZs that *have* been fixed will all report RESOLVED|FIXED,
because TianoCore contributors and maintainers diligently close BZs as
FIXED when the corresponding patches are merged. They even mention the
commit range(s) implementing the related code changes, without fail.
- Once you have your set of Really Fixed (TM) upstream BZs, run a search
in your downstream tracker to locate the referring downstream tickets,
checking the "See Also" (etc) fields.
In a more serious tone: while Red Hat preaches and practices "upstream
first", we obviously *do* have downstream tickets for bugfixes and
features. And if we are *inheriting* patches for them via a rebase (as
opposed to backporting / cherry-picking them), then we benefit from the
same kind of linkage. That's why I keep "lecturing" maintainers when
they fail to close BZs, and/or to note the subject commit ranges (which
I might want to investigate manually).
Now, I realize that "git forges" can auto-close tickets when
encountering ticket references in merged patches. The problem is that
*multiple* patches may reference a ticket and *still* not constitute a
complete fix for that ticket -- see my "multiple waves of patches" note
above. Automation cannot fully supplant manual ticket wrangling.
NB, the above procedure could also help with composing the "feature
list" for any upcoming edk2 stable tag. When collecting the URLs, and
checking their fresh statuses, also check the "Product" fields. If
Product is "TianoCore Feature Requests", then the ticket is a good
candidate to name at