On 4 February 2016 at 21:00, Carsey, Jaben jaben.carsey@intel.com wrote:
-----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, February 04, 2016 12:23 PM To: Ryan Harkin ryan.harkin@linaro.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org; Justen, Jordan L jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Leif Lindholm leif.lindholm@linaro.org; linaro-uefi@lists.linaro.org; Carsey, Jaben jaben.carsey@intel.com Subject: Re: [edk2] [PATCH v2 6/6] Revert "ArmPlatformPkg: Create an ARM Platform DSC / FDF / ArmPlatformLib template" Importance: High
On 02/04/16 12:53, Ryan Harkin wrote:
On 3 February 2016 at 17:38, Leif Lindholm leif.lindholm@linaro.org
wrote:
On Wed, Feb 03, 2016 at 05:29:58PM +0000, Carsey, Jaben wrote:
-----Original Message----- From: Ryan Harkin [mailto:ryan.harkin@linaro.org] Sent: Wednesday, February 03, 2016 9:25 AM To: Carsey, Jaben jaben.carsey@intel.com Cc: edk2-devel@ml01.01.org; linaro-uefi@lists.linaro.org; Leif Lindholm leif.lindholm@linaro.org; Justen, Jordan L
jordan.l.justen@intel.com; Ard
Biesheuvel ard.biesheuvel@linaro.org Subject: Re: [edk2] [PATCH v2 6/6] Revert "ArmPlatformPkg: Create an
ARM
Platform DSC / FDF / ArmPlatformLib template" Importance: High
On 3 February 2016 at 17:20, Carsey, Jaben jaben.carsey@intel.com wrote: > Change looks good. > > Question - Is there a way to optimize the patch/email for this type of
change
so that we do not see the "diff" for a file that is deleted? It seems
redundant
to show a whole file of "-" lines... >
That's a good question. I remember Ard discussing this previously. I'll see if I can find the discussion...
Thanks! That would be good.
I can't find Ard's original message. But I've worked out that if I do
$ git format-patch -D -1 <sha>
It will not show all the deleted lines. I could then "git send-email" the patch file, rather than the using the SHA.
However, the patch will not be able to be applied because it's missing the delete of the contents. So it's only useful for review, unfortunately. I vaguely remember Ard mentioning that in his original email - that I can't find!
I don't think that -D is a good idea. The patch should be applicable from the mailing list for testing & local review.
I agree. It was the only thing I could find and I haven't used it.
I do use a number of tweaks that procude more readable patches:
(1) I enable rename and copy detection. See "diff.renames" in git-config(1).
(2) When appropriate, I even force copy detection for patches where the original files (being copied from) are not touched in the same patch. See "--find-copies-harder" in git-diff(1).
(3) Earlier we discussed how section headers of dec, dsc, inf etc. files can be captured in diff hunk header. Most recently: please see the end of the message at
http://thread.gmane.org/gmane.comp.bios.edk2.devel/7156/focus=7183
(4) I use the "patience" diff algorithm. See "diff.algorithm" in git-config(1), and git-diff(1). A good explanation can be found in the QEMU wiki, under
Make code motion patches easy to review
http://wiki.qemu.org/Contribute/SubmitAPatch#Make_code_motion_patches _easy_to_review
(5) I order the files in my diffs so that the descriptive files go first (dec, dsc, fdf, inf), the C language header files, and HII forms, second, and the C source code last. In my experience, seeing the header changes before the C code changes makes a huge difference.
For this, create a text file like:
*.dec *.dsc.inc *.dsc *.fdf *.inf *.h *.vfr
*.c
That's generally a good idea.
Occasionally, I change pairs of .dsc and .fdf files at the same time and unfortunately this will split them. Eg:
This:
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc ArmPlatformPkg/ArmPlatformPkg-2ndstage.fdf ArmPlatformPkg/ArmPlatformPkg.dsc ArmPlatformPkg/ArmPlatformPkg.fdf
Would become this:
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc ArmPlatformPkg/ArmPlatformPkg.dsc ArmPlatformPkg/ArmPlatformPkg-2ndstage.fdf ArmPlatformPkg/ArmPlatformPkg.fdf
So I guess the I just need to apply a bit of intelligence to when I use the order file.
and pass the pathname of this text file to git-format-patch with the "-O" option (no space between option and option argument).
(6) Occasionally I increase the context from 3 lines to 5 or so (with "-U5").
None of these solve the exact request formulated by Jaben, but they all keep the patch applicable from the mailing list.
You're right that these don't fix the issue that I brought up. My impression is that there is no direct solution so far.
That was my conclusion too.
But these are all excellent ideas and methods to improve the usability of the repo. I think that as much of Laszlo's idea #1, #2, #3, and #4 above can be used will be really helpful.
Agreed. I've already implemented most of the in my tree.
-Jaben
Thanks Laszlo
The worst example is the patch with a delete and then some code change
(probably should be a series).
You can tell this from the start of the diff for each file:
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc +++ /dev/null ,
And also from the diffstat (if all '-' signs next to a file, nothing was added, if anything was added, you'll see '+' also).
>> ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 382 --------------
>> ArmPlatformPkg/ArmPlatformPkg-2ndstage.fdf | 263 ------------ >> ArmPlatformPkg/ArmPlatformPkg.dsc | 445 --------------------- >> ArmPlatformPkg/ArmPlatformPkg.fdf | 320 --------------- >> .../ArmPlatformLibNull/ArmPlatformLibNull.c | 162 -------- >> .../ArmPlatformLibNull/ArmPlatformLibNull.inf | 51 --- >> .../ArmPlatformLibNull/ArmPlatformLibNullMem.c | 34 -- >> .../ArmPlatformLibNull/ArmPlatformLibNullSec.inf | 47 ---
And, yes, it should :)
/ Leif
>> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
Behalf Of
>> Ryan Harkin >> Sent: Wednesday, February 03, 2016 9:10 AM >> To: edk2-devel@ml01.01.org; linaro-uefi@lists.linaro.org; Leif
Lindholm
>> leif.lindholm@linaro.org >> Cc: Ryan Harkin ryan.harkin@linaro.org; Ard Biesheuvel >> ard.biesheuvel@linaro.org >> Subject: [edk2] [PATCH v2 6/6] Revert "ArmPlatformPkg: Create an
ARM
>> Platform DSC / FDF / ArmPlatformLib template" >> Importance: High >> >> This reverts commit
12fcdcb83d8e91cb730db9252189269467b9ed73
>> >> The files added by the commit have never been used. In the
intervening
>> years since they've been added, ~30 patches have been submitted to >> update them. >> >> Remove them as part of a general cleanup of ArmPlatformPkg. >> >> Signed-off-by: Ryan Harkin ryan.harkin@linaro.org >> --- >> ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 382 --------------
>> ArmPlatformPkg/ArmPlatformPkg-2ndstage.fdf | 263 ------------ >> ArmPlatformPkg/ArmPlatformPkg.dsc | 445 --------------------- >> ArmPlatformPkg/ArmPlatformPkg.fdf | 320 --------------- >> .../ArmPlatformLibNull/ArmPlatformLibNull.c | 162 -------- >> .../ArmPlatformLibNull/ArmPlatformLibNull.inf | 51 --- >> .../ArmPlatformLibNull/ArmPlatformLibNullMem.c | 34 -- >> .../ArmPlatformLibNull/ArmPlatformLibNullSec.inf | 47 --- >> 8 files changed, 1704 deletions(-) >>
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel