On 4 February 2016 at 21:00, Carsey, Jaben <[email protected]> wrote: >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, February 04, 2016 12:23 PM >> To: Ryan Harkin <[email protected]> >> Cc: Ard Biesheuvel <[email protected]>; Justen, Jordan L >> <[email protected]>; [email protected]; Leif Lindholm >> <[email protected]>; [email protected]; Carsey, Jaben >> <[email protected]> >> 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 <[email protected]> >> wrote: >> >> On Wed, Feb 03, 2016 at 05:29:58PM +0000, Carsey, Jaben wrote: >> >>>> -----Original Message----- >> >>>> From: Ryan Harkin [mailto:[email protected]] >> >>>> Sent: Wednesday, February 03, 2016 9:25 AM >> >>>> To: Carsey, Jaben <[email protected]> >> >>>> Cc: [email protected]; [email protected]; Leif Lindholm >> >>>> <[email protected]>; Justen, Jordan L >> <[email protected]>; Ard >> >>>> Biesheuvel <[email protected]> >> >>>> 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 <[email protected]> >> >>>> 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:[email protected]] On >> Behalf Of >> >>>>>> Ryan Harkin >> >>>>>> Sent: Wednesday, February 03, 2016 9:10 AM >> >>>>>> To: [email protected]; [email protected]; Leif >> Lindholm >> >>>>>> <[email protected]> >> >>>>>> Cc: Ryan Harkin <[email protected]>; Ard Biesheuvel >> >>>>>> <[email protected]> >> >>>>>> 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 <[email protected]> >> >>>>>> --- >> >>>>>> 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 >> > [email protected] >> > https://lists.01.org/mailman/listinfo/edk2-devel >> > >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
