Hi Wadhawan,

I will check it after August because I am busy recently.

Best Regards
Guomin

On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.

(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.

Laszlo,

Your wiki article was very very helpful. I just could not find it from the Tianocre wiki. It would be good if we could link to it from here [1], maybe as add to this: "Are you new to using git? If so, then the New to git page may be helpful."?

There are a lot folks who use git but don't use the email based review so they have never setup git with emali before. Your wiki, plus me figuring out the magic internal SMTP reflector (I reached out on an internal git malling list) is what got me unblocked.

[1] https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Thanks,

Andrew Fish

Thanks
Laszlo

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.

(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.

Thanks
Laszlo

On May 21, 2020, at 10:48 PM, Bret Barkelew <Bret.Barkelew@microsoft.com> wrote:

“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.

Bret,

Sorry for being confusing, and lazy.....

The lazy part is in house we have a bug tracking tool called radar, so I just replaced radar with BZ to make a general point.

So the scripts I mentioned are useless for us as they 100% rely on massive amounts of internal framework, and are hard coded to our current process.

Probably better to tell the story... So we revamped our internal process and that was lead by the folks who make the OS (so kind of like our edk2 process derived from the Linux kernel). So building the OS made sense, but developers got stuck doing a bunch of manual work. The response was to get a group of smart folks together and write good documentation, and build tools to automate common task. That seems like a good plane for TianoCore too?

So I finally tracked down on our internal git mailing and figured out the mail reflector I needed to follow the basic edk2 rules for patches. To me it seems like we could try and automate thing more. I also found I had to Bing/Google to find the detailed instructions I needed as a developer, as the Wiki seems to assume you just know the Linux kernel patch process. That feels like an area we can improve.

So this makes a couple of ideas pop into my head:
1) It would be good for folks that are not conversant in the Linux mailing list process to give feedback on all the Wikis.
2) Can we make a mail reflector that makes it easier to contribute? The hardest thing for me was tracking down my internal git work group that had setup for how to configure a mail server. Is there a way to help others with that?
3) We want to make sure any new process makes it as easy as possible to contribute.

I'm reminded of the epiphany I got reading Code Complete the 1st time. The data shows the most important thing is consistency. So I'd say our reluctance to change in rooted in the science of computer science but progresses is always our goal.

Thanks,

Andrew Fish

<Tangent>
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.

Thanks!

- Bret

From: Andrew Fish <mailto:afish@apple.com>
Sent: Thursday, May 21, 2020 8:00 PM
To: Laszlo Ersek <mailto:lersek@redhat.com>
Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; spbrogan@outlook.com <mailto:spbrogan@outlook.com>; rfc@edk2.groups.io <mailto:rfc@edk2.groups.io>; Desimone, Nathaniel L <mailto:nathaniel.l.desimone@intel.com>; Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>; Kinney, Michael D <mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) <mailto:leif@nuviainc.com>
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@redhat.com> wrote:

On 05/20/20 00:25, Sean wrote:

On 5/19/2020 1:41 PM, Laszlo Ersek wrote:

Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.

You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".

Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.

The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.

And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.

For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.

Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.

If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.

I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.

I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.


The mailing list equivalent of "squash-on-merge" would be the following:

- contributor posts v1 with patches 1/5 .. 5/5 (for example),

- reviewer requests updates A, B, and C,

- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8

- reviewer checks the new patches and approves them, functionally,

- maintainer says "OK let me merge this",

- maintainer applies the patches (all 8 of them) from the list, on a
local branch,

- maintainer runs a git rebase squashing the whole thing into a single
patch,

- maintainer does *not* review the result,

- maintainer opens a PR with the resultant single patch,

- CI passes,

- the patch is merged.


With the list-based process, the disaster in the last step is mitigated
in at least three spots:

- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.

- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.

- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.

(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)


If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.

It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".

I do think those rules will need to be defined but that is needed
today anyway.

Rules are only as good as their enforcement is.

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 the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).

I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.

I'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.

Thanks,

Andrew Fish

Thanks,
Laszlo

On 05/22/20 07:48, Bret Barkelew wrote:

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.

Proposal:

- 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.
Right? :)

- 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
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#proposed-features>.

Thanks,
Laszlo

On 5/15/20 1:34 AM, Laszlo Ersek wrote:

On 05/14/20 23:26, Bret Barkelew via groups.io wrote:

It’s code management for the Instagram generation

I find this an extremely good characterization!

And, I find the fact soul-destroying.

I was working on a web project recently, and apparently people don't even check email any more! So someone had set up a Slack channel where Github pull requests were posted/linked, and we were supposed to react with thumbs-up, "OK" etc. emoji to indicate we'd seen/reviewed/accepted the request.


--

Rebecca Cran

On 5/14/20 3:26 PM, Bret Barkelew via groups.io wrote:

I feel like this process is a good compromise. It’s not perfect (frankly, I’m a fan of enforced squash merges, which can maintain bisectability if managed well), but it allows for rapid iteration, ease of contribution, and approaches the workflow that many who have never used email to maintain a project would be familiar with.

It’s code management for the Instagram generation, and I for one welcome our new insect overlords.

Or at least, that's what Microsoft is betting on! :D

Personally, I remain unconvinced about the usability of Github Pull Requests for a project the size of EDK2, but I hope to be proven wrong.


--
Rebecca Cran

Mike,

On 5/10/20 3:29 PM, Michael D Kinney wrote:

There is no difference between CI checks run during code review
and the final CI checks before merge. I think it is an interesting
conversation to decide how many times those CI checks should be
run and if they should run automatically on every change during
review or on demand.

I'd suggest following what other Github projects do, which I think is to run the CI checks automatically on every change that's made in a pull request - I don't know if it might also be necessary to run them during the merge, if master has changed in the meantime. That gives the _submitter_ feedback about any changes they need to make, instead of having to wait until the maintainer tells them their change has broken something: it speeds up the development process.

Mergify is more flexible. We want to make sure the git history
is linear with not git merges and supports both single patches
and patch series without squashing. GitHub merge button by
default squashes all commits into a single commit.

Wouldn't disabling all but "Allow rebase merging" do the same thing without the additional potential failure point? Though it sounds like we've resolved the problems with Mergify, so it's not important.

https://help.github.com/en/github/administering-a-repository/configuring-commit-squashing-for-pull-requests


--
Rebecca Cran

On 5/8/20 8:59 PM, Michael D Kinney wrote:

* Perform final review of patches and commit message tags. If there are not
issues, set the `push` label to run final set of CI checks and auto merge
the pull request into master.

What's the difference between the CI that runs when a user submits the Pull Request, and the final CI checks that run before the request is merged?

Also, I'm wondering why Mergify is being used instead of the maintainer hitting the "Merge Pull Request" button, or however it's worded?


--
Rebecca Cran

“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.

<Tangent>
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.

Thanks!

- Bret

From: Andrew Fish<mailto:afish@apple.com>
Sent: Thursday, May 21, 2020 8:00 PM
To: Laszlo Ersek<mailto:lersek@redhat.com>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
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@redhat.com> wrote:

On 05/20/20 00:25, Sean wrote:

On 5/19/2020 1:41 PM, Laszlo Ersek wrote:

Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.

You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".

Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.

The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.

And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.

For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.

Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.

If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.

I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.

I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.


The mailing list equivalent of "squash-on-merge" would be the following:

- contributor posts v1 with patches 1/5 .. 5/5 (for example),

- reviewer requests updates A, B, and C,

- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8

- reviewer checks the new patches and approves them, functionally,

- maintainer says "OK let me merge this",

- maintainer applies the patches (all 8 of them) from the list, on a
local branch,

- maintainer runs a git rebase squashing the whole thing into a single
patch,

- maintainer does *not* review the result,

- maintainer opens a PR with the resultant single patch,

- CI passes,

- the patch is merged.


With the list-based process, the disaster in the last step is mitigated
in at least three spots:

- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.

- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.

- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.

(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)


If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.

It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".

I do think those rules will need to be defined but that is needed
today anyway.

Rules are only as good as their enforcement is.

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 the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).

I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.

I'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.

Thanks,

Andrew Fish

Thanks,
Laszlo

You know what…

That’s fair.
Apologies to the community.

- Bret

From: Laszlo Ersek<mailto:lersek@redhat.com>
Sent: Wednesday, May 20, 2020 2:53 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

off-topic, but for the record:

On 05/19/20 22:10, Bret Barkelew via groups.io wrote:

In my history with TianoCore, I have learned to not be so quick to
say “this is fucking stupidâ€&#65533;. Every time I’ve done that, I’ve later
discovered the reasons behind it, and even come to the conclusion
that the designers were quite clever.

while I understand and appreciate the positive message here, that
particular present participle stands out to me like a sore thumb.

I couldn't resist, and I searched my edk2-devel archives for it (for the
four letter stem, that is), which go back to ~April 2012.

I'm reporting that in all these years, this has indeed been the first
use of the word.

(Not counting the base64 encodings of some binary files that were posted
to the list, in patches -- but those encodings hardly contain "words".)

Can we stay civil, please?

(And no, I'm not a prude; in fact I've shown such restraint in my own
word choices on this list that I can only congratulate myself.)

Thanks,
Laszlo

On May 21, 2020, at 6:30 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 05/20/20 00:25, Sean wrote:

On 5/19/2020 1:41 PM, Laszlo Ersek wrote:

Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.

You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".

Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.

The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.

And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.

For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.

Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.

If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.

I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.

I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.


The mailing list equivalent of "squash-on-merge" would be the following:

- contributor posts v1 with patches 1/5 .. 5/5 (for example),

- reviewer requests updates A, B, and C,

- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8

- reviewer checks the new patches and approves them, functionally,

- maintainer says "OK let me merge this",

- maintainer applies the patches (all 8 of them) from the list, on a
local branch,

- maintainer runs a git rebase squashing the whole thing into a single
patch,

- maintainer does *not* review the result,

- maintainer opens a PR with the resultant single patch,

- CI passes,

- the patch is merged.


With the list-based process, the disaster in the last step is mitigated
in at least three spots:

- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.

- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.

- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.

(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)


If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.

It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".

I do think those rules will need to be defined but that is needed
today anyway.

Rules are only as good as their enforcement is.

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 the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).

I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.

I'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.

Thanks,

Andrew Fish

Thanks,
Laszlo

On May 20, 2020, at 10:21 AM, Sean <spbrogan@outlook.com> wrote:

When this is done in a PR with branch protections this works out differently and in my view your concerns are mitigated.

1. There isn't a partial squash operation. All reviewers know that the final output of the PR is going to 1 commit. Thus there is no confusion of what or how it is being committed to the target branch.

I use Stash/Bitbucket but even the UI is biased towards this. There is an Overview, Diff, and Commits tab. The default diff is the entire PR, you can go to commits tab and see a list of the commits/patch set and see only the diffs for those. I'm not sure how github does it.

In our world we don't require the squash. We also have a set of command line tools that help automate common operations.

Thanks,

Andrew Fish

2. With GitHub branch protections requiring the PR only being merged if it is up-to-date with the target branch. This means you have to push the button in github to merge in target and if any conflicts occur the PR is flagged and can't be completed without user involvement. This would also give reviewers an opportunity to review the merge commit if necessary.

3. With GitHub status checks and branch policies correctly configured the builds are re-run every time the target branch changes. This means that if you have confidence in your PR gates catching most practical merge errors (at least the ones the submitter would catch) you have avoided this issue. This is why the PR builds check every thing in the tree rather than just the incoming patch.

Again, this ask was not to create a lazy process or lower the quality of the code tree. If there are legitimate gaps that a squash merge workflows creates, I am interested in finding solutions. For example, the DCO requirement would need to be addressed. But we can only start those conversations if we can get aligned on the idea.

Thanks
Sean




On 5/20/2020 10:05 AM, Laszlo Ersek wrote:

On 05/19/20 23:02, Desimone, Nathaniel L wrote:

Of course, there may be other patch series that would be logical to
squash, especially if the author has not been careful to maintain
bisectability. For example, I think of some patch series went a
little overboard and could have been done in maybe 1-2 patches
instead of 8-10. I would be happy to compromise with you and say that
squashes can be done in circumstances where both the maintainer and
the author agree to it.

Important distinction:
(a) "squashing patches" is a 100% valid operation that some situations
fully justifiedly call for. Maintainers may ask for it, and contributors
may use it with or without being asked, if the situation calls for it.
(b) "squashing patches *on merge*" is intolerable.
The difference is whether there is a final human review for the
*post-squash* state before the merge occurs.
The valid case is when the contributor squashes some patches, resubmits
the review/pull request, the reviewer approves the *complete* work
(after performing another review, which may of course be incremental in
nature), and then the series is merged exactly as it was submitted.
The invalid case (squash on merge) is when the reviewer checks /
approves the series when it still contains incremental fixes as
broken-out patches, then squashes some patches (in the worst case: all
patches into one), and then merges the result. In this (invalid) case,
the complete work, in its final state (in the way it's going to land in
the git history) has not been reviewed by either submitter or reviewer,
incrementally or otherwise. This is why squash on merge is intolerable:
it places a sequence of commits into the git history that has never been
reviewed *verbatim* by either submitter or reviewer. It's a "blind
merge", to make up another term for illustration
Squashing is a 100% valid tool, I use it all the time. Squash-on-merge
is a catastrophic process failure.
Thanks
Laszlo

Laszlo,

I appreciate the back and forth. I find email a challenge for this type of discussion because it leaves so much to individual interpretation and bias. Anyway Thank you for having the discussion. I hope others with opinions feel empowered to chime in and help this RFC go in the direction the community and stewards desire.

I am still in full support of the RFC and am ok to table my concerns that changing the tools without adapting the process will lead to a less than optimal workflow. Anyway, I look forward to seeing if the "pull request based code review process" can help improve the communities collaboration and efficiency.

I have a few additional responses below that will clarify my thoughts but hopefully not invoke responses. :)

On 5/21/2020 6:30 AM, Laszlo Ersek wrote:

On 05/20/20 00:25, Sean wrote:

On 5/19/2020 1:41 PM, Laszlo Ersek wrote:

Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.

You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".
Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.
The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.
And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.
For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.

Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.

If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.
I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.
I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.
The mailing list equivalent of "squash-on-merge" would be the following:
- contributor posts v1 with patches 1/5 .. 5/5 (for example),
- reviewer requests updates A, B, and C,
- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8
- reviewer checks the new patches and approves them, functionally,
- maintainer says "OK let me merge this",
- maintainer applies the patches (all 8 of them) from the list, on a
local branch,
- maintainer runs a git rebase squashing the whole thing into a single
patch,
- maintainer does *not* review the result,
- maintainer opens a PR with the resultant single patch,
- CI passes,
- the patch is merged.

The above example should not be allowed in any process.
If a contribution was submitted as a patch series with 5 patches intentionally, then it would not be a candidate for a squash merge. The squash merge workflow is only acceptable when it is agreed that the end result should be 1 patch.

With the list-based process, the disaster in the last step is mitigated
in at least three spots:
- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.
- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.
- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.
(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)
If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.
It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".

I do think those rules will need to be defined but that is needed
today anyway.

Rules are only as good as their enforcement is.
The question is not how nice it is to use squash-on-merge in the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

At time of writing i don't know any way to enforce this if the maintainers can not be relied upon. Given my strong agreement with "Rules are only as good as their enforcement is." I don't see a practical way to resolve this and you seem content with the current solution. Thanks for your diligence here.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).
I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.
Thanks,
Laszlo

Thanks
Sean

On 05/20/20 00:25, Sean wrote:

On 5/19/2020 1:41 PM, Laszlo Ersek wrote:

Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.

You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".

Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.

The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.

And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.

For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.

Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.

If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.

I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.

I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.


The mailing list equivalent of "squash-on-merge" would be the following:

- contributor posts v1 with patches 1/5 .. 5/5 (for example),

- reviewer requests updates A, B, and C,

- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8

- reviewer checks the new patches and approves them, functionally,

- maintainer says "OK let me merge this",

- maintainer applies the patches (all 8 of them) from the list, on a
local branch,

- maintainer runs a git rebase squashing the whole thing into a single
patch,

- maintainer does *not* review the result,

- maintainer opens a PR with the resultant single patch,

- CI passes,

- the patch is merged.


With the list-based process, the disaster in the last step is mitigated
in at least three spots:

- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.

- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.

- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.

(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)


If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.

It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".

I do think those rules will need to be defined but that is needed
today anyway.

Rules are only as good as their enforcement is.

The question is not how nice it is to use squash-on-merge in the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).

I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.

Thanks,
Laszlo

off-topic, but for the record:

On 05/19/20 22:10, Bret Barkelew via groups.io wrote:

In my history with TianoCore, I have learned to not be so quick to
say “this is fucking stupidâ€&#65533;. Every time I’ve done that, I’ve later
discovered the reasons behind it, and even come to the conclusion
that the designers were quite clever.

while I understand and appreciate the positive message here, that
particular present participle stands out to me like a sore thumb.

I couldn't resist, and I searched my edk2-devel archives for it (for the
four letter stem, that is), which go back to ~April 2012.

I'm reporting that in all these years, this has indeed been the first
use of the word.

(Not counting the base64 encodings of some binary files that were posted
to the list, in patches -- but those encodings hardly contain "words".)

Can we stay civil, please?

(And no, I'm not a prude; in fact I've shown such restraint in my own
word choices on this list that I can only congratulate myself.)

Thanks,
Laszlo

When this is done in a PR with branch protections this works out differently and in my view your concerns are mitigated.

1. There isn't a partial squash operation. All reviewers know that the final output of the PR is going to 1 commit. Thus there is no confusion of what or how it is being committed to the target branch.

2. With GitHub branch protections requiring the PR only being merged if it is up-to-date with the target branch. This means you have to push the button in github to merge in target and if any conflicts occur the PR is flagged and can't be completed without user involvement. This would also give reviewers an opportunity to review the merge commit if necessary.

3. With GitHub status checks and branch policies correctly configured the builds are re-run every time the target branch changes. This means that if you have confidence in your PR gates catching most practical merge errors (at least the ones the submitter would catch) you have avoided this issue. This is why the PR builds check every thing in the tree rather than just the incoming patch.

Again, this ask was not to create a lazy process or lower the quality of the code tree. If there are legitimate gaps that a squash merge workflows creates, I am interested in finding solutions. For example, the DCO requirement would need to be addressed. But we can only start those conversations if we can get aligned on the idea.

Thanks
Sean

On 05/19/20 23:02, Desimone, Nathaniel L wrote:

Of course, there may be other patch series that would be logical to
squash, especially if the author has not been careful to maintain
bisectability. For example, I think of some patch series went a
little overboard and could have been done in maybe 1-2 patches
instead of 8-10. I would be happy to compromise with you and say that
squashes can be done in circumstances where both the maintainer and
the author agree to it.

Important distinction:

(a) "squashing patches" is a 100% valid operation that some situations
fully justifiedly call for. Maintainers may ask for it, and contributors
may use it with or without being asked, if the situation calls for it.

(b) "squashing patches *on merge*" is intolerable.

The difference is whether there is a final human review for the
*post-squash* state before the merge occurs.

The valid case is when the contributor squashes some patches, resubmits
the review/pull request, the reviewer approves the *complete* work
(after performing another review, which may of course be incremental in
nature), and then the series is merged exactly as it was submitted.

The invalid case (squash on merge) is when the reviewer checks /
approves the series when it still contains incremental fixes as
broken-out patches, then squashes some patches (in the worst case: all
patches into one), and then merges the result. In this (invalid) case,
the complete work, in its final state (in the way it's going to land in
the git history) has not been reviewed by either submitter or reviewer,
incrementally or otherwise. This is why squash on merge is intolerable:
it places a sequence of commits into the git history that has never been
reviewed *verbatim* by either submitter or reviewer. It's a "blind
merge", to make up another term for illustration

Squashing is a 100% valid tool, I use it all the time. Squash-on-merge
is a catastrophic process failure.

Thanks
Laszlo

Are there any additional comments on the code first process for UEFI specifications?

When should we expect the process to *actually* start being used?

Thanks,
--Samer