Hi On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek <ler...@redhat.com> wrote: > Hi Marc-André, > > On 05/15/18 14:30, marcandre.lur...@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> Hi, >> >> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU >> with TPM2 (I haven't tested TPM1, for lack of interest). > > I got the review of this patch series added to my TODO list, but I'll > have to ask for your patience. :( > > From an extremely superficial skim: > > * please use the > > TopDirPkg/ModuleName: blah blah blah > > subject format, or more generally, if a module cannot be identified, > > TopDirPkg: blah blah blah >
done > * the subject line and the commit message shouldn't be wider than 74 > chars; > that should be ok > * edk2 uses two spaces for general indentation, and I'm noticing some > inconsistency there (4 spaces like in QEMU). yes, I tried to respect that, but sometime fail (emacs c-basic-offset 2 isn't great with comments) > > * Please consider formatting the patches with "--find-copies-harder" > (although I can look at them with the same option after fetching the > series from your repo). This option is usually helpful for reviewers > when cloning and modifying modules cross-package. Hmm, I didn't know that option, ok > > * Please consider adopting the git settings at > > <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>, > in particular: > > - "--stat=1000 --stat-graph-width=20", so that pathnames are not > truncated in the diffstats, > I use git-publish very often. I had to modify it to pass those options (https://github.com/stefanha/git-publish/pull/48) > - the "xfuncname"-related settings, so that git diff hunk headers @@ > are useful for DSC and INF files too, > This is already in my .git/config, I hope it takes it by default in format-patch? > - the diff order file, so that files are listed in patches in logical > order, going from abstract / descriptive (.inf, .h) to concrete / > imperative (.c). > ok > Not much of a review, I know; this is all I can offer right now. If you > have the time to respin just with these superficial changes, that might > make my life easier. If you prefer to delay them, that's 100% fine too. > I am going to resend with the style fixes.
