derailing into patch style discussion

[Leif Lindholm <leif.lindholm@linaro.org>]

Since you already have my r-b on the set, I'll pick up the style
topic, partly because I'm not sure if I've ever explained my
thinking publicly in words that anyone other than Ard understands.

On Thu, Apr 12, 2018 at 07:45:19PM +0200, Laszlo Ersek wrote:
> > Well, there are a couple of places where I could nitpick on
> > alphabetical sorting of includes,
> 
> And, believe me, you would have my total agreement :), but in such
> cases, there's always a fork in the road: (a) add a separate patch that
> first sorts the includes and [LibraryClasses], without functional
> changes, or (b) just stick with the existing disorder, and get in and
> out as surgically as possible. My personal preference is (a), but it has
> drawn disagreement -- even accusations of pedantry :) :) --, and/or
> suggestions to squash such patches with functional changes, in the past,
> so I trod more lightly now. Rest assured, I didn't *miss* those places,
> I just elected to close my eyes! ;)

Right :)

So, yes - I do strongly agree with the idea of keeping functionality
and style changes separate (ask Evan), so that wasn't exactly what I
was referring to.

In general my internal "optimal" situation is one where the purely
functional diff leaves the code in a (quite subjectively, since it's
still not conformant) better state on average than it was.

My subjective mark of optimal is that which would minimise any
subsequent patch that was _only_ a style cleanup.

To pick an example from this set (1/10):
diff --git a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
index 6deb8c3f675c..61ad89af2758 100644
--- a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
+++ b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
@@ -41,14 +41,14 @@ [LibraryClasses]
   PrintLib
   UefiDriverEntryPoint
   IoLib
   ArmLib

 [Protocols]
 -  gHardwareInterruptProtocolGuid
 -  gEfiCpuArchProtocolGuid
 +  gHardwareInterruptProtocolGuid  ## PRODUCES
 +  gEfiCpuArchProtocolGuid         ## CONSUMES ## NOTIFY

---
This one is pretty straightforward - without touching any non-modified
lines, these _could_ be reordered alphabetically.

(Yes, that change may have functional side-effects, but only on
undefined behaviour, so no different from rebasing to a version with
newer BaseTools can give you.)

Where the minimum diff logic comes in is in situations like (6/10):
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 812dafd065b2..c40ac27a6599 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -28,12 +28,13 @@ [Sources.common]
   NorFlashBlockIoDxe.c

 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec

---
Applying my "optimal" rule here would have meant inserting the new
line before MdePkg/MdeModulePkg .decs instead. That way a cleanup
patch would end up doing

[Packages]
+  ArmPlatformPkg/ArmPlatformPkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
-  ArmPlatformPkg/ArmPlatformPkg.dec

instead of

[Packages]
-  MdePkg/MdePkg.dec
-  MdeModulePkg/MdeModulePkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec


Anyway, this is all an aside - I just thought I would give you an
insight into the mind of Leif.

/
    Leif