On Tue, Nov 14, 2023 at 3:01 PM Laszlo Ersek <ler...@redhat.com> wrote: > > On 11/13/23 22:33, Pedro Falcato wrote: > > On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran <rebe...@bsdio.com> wrote: > >> > >> On 11/13/2023 1:08 PM, Michael Kubacki wrote: > >>> Yes. I just did it. It is relatively minor and impacts expected code > >>> areas. > >>> > >>> https://github.com/tianocore/edk2/pull/5043/files > >> > >> > >> Could you update .git-blame-ignore-revs please? > > > > You can't do that until the merge is done, since we use > > rebase-and-merge for tianocore (and rebases do not keep stable commit > > hashes). > > But I would plead that this should not get merged in general :/ >
Laszlo! Sorry for the delay. > Seeing the cumulative diff in that PR, do you have specific > counter-arguments? Well, my counter-argument is that formatting is becoming a topic of its own. I used to be very pro-formatter but if this leads to either 1) eyesore or 2) weird churn every now and then, I feel like we should reconsider the current approach. I feel like all formatting (manual or automated) is fine as long as it's: 1) Visually consistent with the codebase's style 2) Not horrendous to look at and switching back and forth because 'magic indentation tool' says so just seems silly to me. > > The diff is trivial, IMO. You mentioned "error prone" and "much churn", > which are very valid concerns, but they don't seem to apply here. We can > review a diff of this size (especially if it's split up on Pkg > boundaries), and the github view indicates the change is only in > whitespace amount. > > The change in > "OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c" > is a net win; the current formatting is really distracting. > > Furthermore, this diff actually highlights some inexplicable syntax in > "EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c": those backslashes (not > parts of any macro definition) are an eyesore. They should be fixed > regardless of re-uncrustification. > > The version N vs. N+1 concern shouldn't be one; the authoritative > version is what the YAML file in edk2 says. Well, I fear it's not that simple. EDK2's uncrustify has historically been a PITA, I've had to convince people to give it a try, I myself don't even know how I installed it (IIRC, some weird random combination of unzip + the NuGet...). Getting everyone on the same version would already be hard even if the software was easy to install. -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111304): https://edk2.groups.io/g/devel/message/111304 Mute This Topic: https://groups.io/mt/102559740/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
