Laszlo Ersek
2015-06-09 17:37:46 UTC
(Sorry about the sensationalist subject line, I don't have a degree in
marketing :))
Edk2 uses a large number of text files with *sections*:
[Section.LOL]
I'm sure you've been annoyed quite a few times, while reviewing patches,
that you couldn't immediately see the section that a hunk modified. (I
know I have.) We used to have two solutions for this:
- On the reviewer side, apply the patchset and review it patch-wise,
against the full source code as context.
- On the sender side, generate the patches with a larger context.
Options are -U<n>, --inter-hunk-context=<lines>, and even
--function-context. Unfortunately, these don't resolve the question of
sections reliably (or they produce overkill output).
Turns out git has a trick for this up its sleeve: see gitattributes(5).
(1) Edit your .git/info/attributes file, adding the following lines:
*.dec diff=ini
*.dsc diff=ini
*.dsc.inc diff=ini
*.fdf diff=ini
*.fdf.inc diff=ini
*.inf diff=ini
(2) in your .git/config, add
[diff "ini"]
xfuncname = "^\\[[A-Za-z0-9_., ]+]"
I just set these in my edk2 clone, and tested them on my local branch
where I had applied Ard's series
[PATCH 0/7] ArmPkg/ArmVirtPkg: GIC revision detection
for review. A good example is patch #4, "ArmPkg: copy ArmGicArchLib to
ArmGicArchSecLib". Now I can immediately see, in the hunk headers (@@),
what section each change belongs to.
for SEC modules, a fact that was not visible otherwise from the patch
itself.
I recommend that all git users working with edk2 apply these settings --
they are *very* helpful for reviewers. Personally, I will make this a
requirement for patches that I'm expected (or asked) to review.
Thanks
Laszlo
------------------------------------------------------------------------------
marketing :))
Edk2 uses a large number of text files with *sections*:
[Section.LOL]
I'm sure you've been annoyed quite a few times, while reviewing patches,
that you couldn't immediately see the section that a hunk modified. (I
know I have.) We used to have two solutions for this:
- On the reviewer side, apply the patchset and review it patch-wise,
against the full source code as context.
- On the sender side, generate the patches with a larger context.
Options are -U<n>, --inter-hunk-context=<lines>, and even
--function-context. Unfortunately, these don't resolve the question of
sections reliably (or they produce overkill output).
Turns out git has a trick for this up its sleeve: see gitattributes(5).
(1) Edit your .git/info/attributes file, adding the following lines:
*.dec diff=ini
*.dsc diff=ini
*.dsc.inc diff=ini
*.fdf diff=ini
*.fdf.inc diff=ini
*.inf diff=ini
(2) in your .git/config, add
[diff "ini"]
xfuncname = "^\\[[A-Za-z0-9_., ]+]"
I just set these in my edk2 clone, and tested them on my local branch
where I had applied Ard's series
[PATCH 0/7] ArmPkg/ArmVirtPkg: GIC revision detection
for review. A good example is patch #4, "ArmPkg: copy ArmGicArchLib to
ArmGicArchSecLib". Now I can immediately see, in the hunk headers (@@),
what section each change belongs to.
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 0859bc3..6e6687c 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025..14d82f6 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@ [LibraryClasses.common.SEC]
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de381..4b4867f 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a99..8f7b5f1 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@ [LibraryClasses.common.SEC]
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
This git-diff output shows quickly that Ard resolved the library classindex 0859bc3..6e6687c 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025..14d82f6 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@ [LibraryClasses.common.SEC]
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de381..4b4867f 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a99..8f7b5f1 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@ [LibraryClasses.common.SEC]
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
for SEC modules, a fact that was not visible otherwise from the patch
itself.
I recommend that all git users working with edk2 apply these settings --
they are *very* helpful for reviewers. Personally, I will make this a
requirement for patches that I'm expected (or asked) to review.
Thanks
Laszlo
------------------------------------------------------------------------------