| Message ID | 20161202210152.6880-1-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 02, 2016 at 10:01:52PM +0100, Laszlo Ersek wrote: [...] > +docs/* > +*.txt > +configure > +GNUmakefile > +makefile > +Makefile* > +*.mak > +qapi-schema*.json > +qapi/*.json > +include/qapi/visitor.h > +include/qapi/visitor-impl.h > +scripts/qapi.py > +scripts/*.py > +*.h > +qapi/qapi-visit-core.c Maybe we could include test/* here, so test code appears before the implementation? > +*.c > -- > 2.9.2 > > -- Eduardo
On 12/02/16 22:16, Eduardo Habkost wrote: > On Fri, Dec 02, 2016 at 10:01:52PM +0100, Laszlo Ersek wrote: > [...] >> +docs/* >> +*.txt >> +configure >> +GNUmakefile >> +makefile >> +Makefile* >> +*.mak >> +qapi-schema*.json >> +qapi/*.json >> +include/qapi/visitor.h >> +include/qapi/visitor-impl.h >> +scripts/qapi.py >> +scripts/*.py >> +*.h >> +qapi/qapi-visit-core.c > > Maybe we could include test/* here, so test code appears before > the implementation? Hmmm, I'm not so sure about that. I certainly don't subscribe to TDD, when you first write up the test case, and then add implementation to satisfy the tests. I don't have much experience / history with the tests subdir. Briefly reviewing what I've done there (git log --author=lersek), I think I would either keep the test patches entirely separate from the QEMU code patches (thus there wouldn't be a patch modifying *.c files both under and outside of tests/). Or else, I would actually prefer tests/*.c to come later in the same patch. The tests are frequently written to provide good coverage for the actual QEMU code, so seeing the QEMU code first seems preferable. (The corner cases to cover could be completely arbitrary, originating from quirks of POSIX functions, and so on.) Thanks Laszlo >> +*.c >> -- >> 2.9.2 >> >> >
On 12/02/2016 04:54 PM, Laszlo Ersek wrote: > On 12/02/16 22:16, Eduardo Habkost wrote: >> On Fri, Dec 02, 2016 at 10:01:52PM +0100, Laszlo Ersek wrote: >> [...] >>> +docs/* >>> +*.txt >>> +configure >>> +GNUmakefile >>> +makefile >>> +Makefile* >>> +*.mak >>> +qapi-schema*.json >>> +qapi/*.json >>> +include/qapi/visitor.h >>> +include/qapi/visitor-impl.h >>> +scripts/qapi.py >>> +scripts/*.py >>> +*.h >>> +qapi/qapi-visit-core.c >> >> Maybe we could include test/* here, so test code appears before >> the implementation? > > Hmmm, I'm not so sure about that. I certainly don't subscribe to TDD, > when you first write up the test case, and then add implementation to > satisfy the tests. > > I don't have much experience / history with the tests subdir. Briefly > reviewing what I've done there (git log --author=lersek), I think I > would either keep the test patches entirely separate from the QEMU code > patches (thus there wouldn't be a patch modifying *.c files both under > and outside of tests/). Or else, I would actually prefer tests/*.c to > come later in the same patch. The tests are frequently written to > provide good coverage for the actual QEMU code, so seeing the QEMU code > first seems preferable. (The corner cases to cover could be completely > arbitrary, originating from quirks of POSIX functions, and so on.) > > Thanks > Laszlo > >>> +*.c >>> -- >>> 2.9.2 >>> >>> >> > We also don't formally embrace TDD, as any tests that are checked in _MUST_ pass. The patch order by definition then requires the feature first, and then the tests. Personally I like TDD, but I don't think it's appropriate to put tests first in this instance, as we review with an eye for "Will this break the tree right now?" and not "What are we about to add to QEMU?" --js
On Fri, Dec 02, 2016 at 10:01:52PM +0100, Laszlo Ersek wrote: > When passed to git-diff (and to every other git command producing diffs > and/or diffstats) with "-O" or "diff.orderFile", this list of patterns > will place the more declarative / abstract hunks first, while changes to > imperative code / details will be near the end of the patches. This saves > on scrolling / searching and makes for easier reviewing. > > We intend to advise contributors in the Wiki to run > > git config diff.orderFile scripts/git.orderfile > > once, as part of their initial setup, before formatting their first (or, > for repeat contributors, next) patches. > > See the "-O" option and the "diff.orderFile" configuration variable in > git-diff(1) and git-config(1). > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Fam Zheng <famz@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: John Snow <jsnow@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: Stefan Hajnoczi <stefanha@gmail.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2: > - "Makefile" -> "Makefile*" [Gerd] > - add leading comment [Gerd] > - add "docs/*" (note, there are *.txt files outside of docs/, so keeping > those too) [Max, Fam, Eric] > > scripts/git.orderfile | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 scripts/git.orderfile > > diff --git a/scripts/git.orderfile b/scripts/git.orderfile > new file mode 100644 > index 000000000000..3cab16e0505c > --- /dev/null > +++ b/scripts/git.orderfile > @@ -0,0 +1,20 @@ > +# Apply this diff order to your git configuration with the command > +# > +# git config diff.orderFile scripts/git.orderfile > + > +docs/* do we need this now? .txt is just below. > +*.txt > +configure > +GNUmakefile > +makefile > +Makefile* > +*.mak do these rules apply in each directory? I think they do but then I don't think we should list directories below. > +qapi-schema*.json > +qapi/*.json > +include/qapi/visitor.h > +include/qapi/visitor-impl.h > +scripts/qapi.py > +scripts/*.py > +*.h > +qapi/qapi-visit-core.c is the exact order or qapi files that important? I'd rather we stuck to simple wildcards without special casing visitors etc. > +*.c > -- > 2.9.2
On Sun, 12/04 19:33, Michael S. Tsirkin wrote: > > diff --git a/scripts/git.orderfile b/scripts/git.orderfile > > new file mode 100644 > > index 000000000000..3cab16e0505c > > --- /dev/null > > +++ b/scripts/git.orderfile > > @@ -0,0 +1,20 @@ > > +# Apply this diff order to your git configuration with the command > > +# > > +# git config diff.orderFile scripts/git.orderfile > > + > > +docs/* > > do we need this now? .txt is just below. It is useful for non-.txt files there, such as the bitmaps.md. Fam
On Mon, Dec 05, 2016 at 08:30:21AM +0800, Fam Zheng wrote: > On Sun, 12/04 19:33, Michael S. Tsirkin wrote: > > > diff --git a/scripts/git.orderfile b/scripts/git.orderfile > > > new file mode 100644 > > > index 000000000000..3cab16e0505c > > > --- /dev/null > > > +++ b/scripts/git.orderfile > > > @@ -0,0 +1,20 @@ > > > +# Apply this diff order to your git configuration with the command > > > +# > > > +# git config diff.orderFile scripts/git.orderfile > > > + > > > +docs/* > > > > do we need this now? .txt is just below. > > It is useful for non-.txt files there, such as the bitmaps.md. > > Fam Let's add the .md suffix instead? For example, I'm not sure we want .cfg files on top like this. -- MST
On 12/04/16 18:33, Michael S. Tsirkin wrote: > On Fri, Dec 02, 2016 at 10:01:52PM +0100, Laszlo Ersek wrote: >> When passed to git-diff (and to every other git command producing diffs >> and/or diffstats) with "-O" or "diff.orderFile", this list of patterns >> will place the more declarative / abstract hunks first, while changes to >> imperative code / details will be near the end of the patches. This saves >> on scrolling / searching and makes for easier reviewing. >> >> We intend to advise contributors in the Wiki to run >> >> git config diff.orderFile scripts/git.orderfile >> >> once, as part of their initial setup, before formatting their first (or, >> for repeat contributors, next) patches. >> >> See the "-O" option and the "diff.orderFile" configuration variable in >> git-diff(1) and git-config(1). >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Fam Zheng <famz@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: John Snow <jsnow@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@gmail.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> v2: >> - "Makefile" -> "Makefile*" [Gerd] >> - add leading comment [Gerd] >> - add "docs/*" (note, there are *.txt files outside of docs/, so keeping >> those too) [Max, Fam, Eric] >> >> scripts/git.orderfile | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> create mode 100644 scripts/git.orderfile >> >> diff --git a/scripts/git.orderfile b/scripts/git.orderfile >> new file mode 100644 >> index 000000000000..3cab16e0505c >> --- /dev/null >> +++ b/scripts/git.orderfile >> @@ -0,0 +1,20 @@ >> +# Apply this diff order to your git configuration with the command >> +# >> +# git config diff.orderFile scripts/git.orderfile >> + >> +docs/* > > do we need this now? .txt is just below. I covered this exact question in the Notes section of the patch (see above). > >> +*.txt >> +configure >> +GNUmakefile >> +makefile >> +Makefile* >> +*.mak > > do these rules apply in each directory? > I think they do but then I don't think we should list directories > below. I picked the below lines from Eric's feedback. >> +qapi-schema*.json >> +qapi/*.json >> +include/qapi/visitor.h >> +include/qapi/visitor-impl.h >> +scripts/qapi.py >> +scripts/*.py >> +*.h >> +qapi/qapi-visit-core.c > > is the exact order or qapi files that important? > I'd rather we stuck to simple wildcards without > special casing visitors etc. Eric indicated this specific order was helpful for QAPI work. > >> +*.c >> -- >> 2.9.2 I'm concerned that we're getting into bikeshedding with this thread. I think it is counter-productive to try to get every minute detail "right" (for some definition of "right") in the initial commit. *Whatever* we commit at this point will be an improvement relative to the current status; for example, *.h *.c would be an improvement, simply by placing C source / implementation after headers. We don't need (and shouldn't try) to cover everything we can think of in this version. I suggest we go ahead with this posting (or v1), then people can submit whatever improvements they deem fit, from their personal experience. I think the initial version should be minimal and non-controversial. We've already spent a disproportionate amount of time discussing this item. Thanks Laszlo
On 12/04/2016 11:33 AM, Michael S. Tsirkin wrote: >> +++ b/scripts/git.orderfile >> @@ -0,0 +1,20 @@ >> +# Apply this diff order to your git configuration with the command >> +# >> +# git config diff.orderFile scripts/git.orderfile >> + >> +docs/* > > do we need this now? .txt is just below. > >> +*.txt I still think we want docs/* first, at which point this *.txt line currently only adds changes to qdict-test-data.txt (should that file be moved to live under tests/ ?) >> +configure >> +GNUmakefile >> +makefile >> +Makefile* >> +*.mak > > do these rules apply in each directory? My understanding is that git applies similar logic to these lines as it does to .gitignore files: if not anchored, the pattern applies to any file in any directory; or you can start with leading / to force the match to be anchored to a specific directory. > I think they do but then I don't think we should list directories > below. > >> +qapi-schema*.json >> +qapi/*.json >> +include/qapi/visitor.h >> +include/qapi/visitor-impl.h >> +scripts/qapi.py >> +scripts/*.py >> +*.h >> +qapi/qapi-visit-core.c > > is the exact order or qapi files that important? > I'd rather we stuck to simple wildcards without > special casing visitors etc. On the v1 thread, I had pointed out that I had special-cased visitor code in some of my patches, as an example, but I'm not sure that it is special enough to be needed for everyone. I'm fine doing one-off tweaks for patch series where a particular file should come first, without making the official file have to track all those one-off tweaks. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/05/2016 04:24 AM, Laszlo Ersek wrote: > > I picked the below lines from Eric's feedback. > >>> +qapi-schema*.json >>> +qapi/*.json >>> +include/qapi/visitor.h >>> +include/qapi/visitor-impl.h >>> +scripts/qapi.py >>> +scripts/*.py >>> +*.h >>> +qapi/qapi-visit-core.c >> >> is the exact order or qapi files that important? >> I'd rather we stuck to simple wildcards without >> special casing visitors etc. > > Eric indicated this specific order was helpful for QAPI work. In fact, what I would recommend is that we do the minimal file now, and then subsequent series (such as if I have more QAPI work) can add their further tweaks as part of the series where those tweaks aid review. So I'd be just fine omitting the visitor special-casing if it helps get the patch in faster. > I suggest we go ahead with this posting (or v1), then people can submit > whatever improvements they deem fit, from their personal experience. I > think the initial version should be minimal and non-controversial. > > We've already spent a disproportionate amount of time discussing this item. Indeed, especially for something that still requires manual configuration from each developer to make use of it (it's a shame git doesn't yet have automatic support for .gitorderfile, like it does for .gitignore or .gitattributes). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/05/2016 05:24 AM, Laszlo Ersek wrote: > > I suggest we go ahead with this posting (or v1), then people can submit > whatever improvements they deem fit, from their personal experience. I > think the initial version should be minimal and non-controversial. > > We've already spent a disproportionate amount of time discussing this item. > ACK > Thanks > Laszlo > Reviewed-by: John Snow <jsnow@redhat.com>
On 12/02/2016 03:01 PM, Laszlo Ersek wrote: > When passed to git-diff (and to every other git command producing diffs > and/or diffstats) with "-O" or "diff.orderFile", this list of patterns > will place the more declarative / abstract hunks first, while changes to > imperative code / details will be near the end of the patches. This saves > on scrolling / searching and makes for easier reviewing. > > We intend to advise contributors in the Wiki to run > > git config diff.orderFile scripts/git.orderfile > > once, as part of their initial setup, before formatting their first (or, > for repeat contributors, next) patches. Presumably, we'd also want this to be mentioned in HACKING? > > See the "-O" option and the "diff.orderFile" configuration variable in > git-diff(1) and git-config(1). > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Fam Zheng <famz@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: John Snow <jsnow@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: Stefan Hajnoczi <stefanha@gmail.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- Is it time to revive this patch? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/07/17 20:20, Eric Blake wrote: > On 12/02/2016 03:01 PM, Laszlo Ersek wrote: >> When passed to git-diff (and to every other git command producing diffs >> and/or diffstats) with "-O" or "diff.orderFile", this list of patterns >> will place the more declarative / abstract hunks first, while changes to >> imperative code / details will be near the end of the patches. This saves >> on scrolling / searching and makes for easier reviewing. >> >> We intend to advise contributors in the Wiki to run >> >> git config diff.orderFile scripts/git.orderfile >> >> once, as part of their initial setup, before formatting their first (or, >> for repeat contributors, next) patches. > > Presumably, we'd also want this to be mentioned in HACKING? Presumably. :) > >> >> See the "-O" option and the "diff.orderFile" configuration variable in >> git-diff(1) and git-config(1). >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Fam Zheng <famz@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: John Snow <jsnow@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@gmail.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- > > Is it time to revive this patch? Time for whom? :) Cheers, Laszlo
On Fri, Dec 02, 2016 at 22:01:52 +0100, Laszlo Ersek wrote: > When passed to git-diff (and to every other git command producing diffs > and/or diffstats) with "-O" or "diff.orderFile", this list of patterns > will place the more declarative / abstract hunks first, while changes to > imperative code / details will be near the end of the patches. This saves > on scrolling / searching and makes for easier reviewing. > > We intend to advise contributors in the Wiki to run > > git config diff.orderFile scripts/git.orderfile > > once, as part of their initial setup, before formatting their first (or, > for repeat contributors, next) patches. > > See the "-O" option and the "diff.orderFile" configuration variable in > git-diff(1) and git-config(1). > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Fam Zheng <famz@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: John Snow <jsnow@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: Stefan Hajnoczi <stefanha@gmail.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- Refloating 6+ months later, but.. Someone please merge this! :-) E.
On Tue, Jun 27, 2017 at 02:51:52PM -0400, Emilio G. Cota wrote: > On Fri, Dec 02, 2016 at 22:01:52 +0100, Laszlo Ersek wrote: > > When passed to git-diff (and to every other git command producing diffs > > and/or diffstats) with "-O" or "diff.orderFile", this list of patterns > > will place the more declarative / abstract hunks first, while changes to > > imperative code / details will be near the end of the patches. This saves > > on scrolling / searching and makes for easier reviewing. > > > > We intend to advise contributors in the Wiki to run > > > > git config diff.orderFile scripts/git.orderfile > > > > once, as part of their initial setup, before formatting their first (or, > > for repeat contributors, next) patches. > > > > See the "-O" option and the "diff.orderFile" configuration variable in > > git-diff(1) and git-config(1). > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Eric Blake <eblake@redhat.com> > > Cc: Fam Zheng <famz@redhat.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: John Snow <jsnow@redhat.com> > > Cc: Max Reitz <mreitz@redhat.com> > > Cc: Stefan Hajnoczi <stefanha@gmail.com> > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > Refloating 6+ months later, but.. > > Someone please merge this! :-) > > E. Users should be aware that if they do this, the need to supply -O /dev/null to git pull-request. Otherwise the diffstat is all scrambled and you can't figure out what's included. -- MST
On Tue, Jun 27, 2017 at 22:16:39 +0300, Michael S. Tsirkin wrote: > On Tue, Jun 27, 2017 at 02:51:52PM -0400, Emilio G. Cota wrote: > > On Fri, Dec 02, 2016 at 22:01:52 +0100, Laszlo Ersek wrote: > > > When passed to git-diff (and to every other git command producing diffs > > > and/or diffstats) with "-O" or "diff.orderFile", this list of patterns > > > will place the more declarative / abstract hunks first, while changes to > > > imperative code / details will be near the end of the patches. This saves > > > on scrolling / searching and makes for easier reviewing. > > > > > > We intend to advise contributors in the Wiki to run > > > > > > git config diff.orderFile scripts/git.orderfile > > > > > > once, as part of their initial setup, before formatting their first (or, > > > for repeat contributors, next) patches. > > > > > > See the "-O" option and the "diff.orderFile" configuration variable in > > > git-diff(1) and git-config(1). > > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > Cc: Eric Blake <eblake@redhat.com> > > > Cc: Fam Zheng <famz@redhat.com> > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > > Cc: John Snow <jsnow@redhat.com> > > > Cc: Max Reitz <mreitz@redhat.com> > > > Cc: Stefan Hajnoczi <stefanha@gmail.com> > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > > --- > > > > Refloating 6+ months later, but.. > > > > Someone please merge this! :-) > > > > E. > > Users should be aware that if they do this, the need to > supply -O /dev/null to git pull-request. Otherwise the > diffstat is all scrambled and you can't figure out > what's included. I'd do the opposite: recommend passing '-O scripts/git.orderfile' to git-format patch, and leave the default for all other commands. E.
diff --git a/scripts/git.orderfile b/scripts/git.orderfile new file mode 100644 index 000000000000..3cab16e0505c --- /dev/null +++ b/scripts/git.orderfile @@ -0,0 +1,20 @@ +# Apply this diff order to your git configuration with the command +# +# git config diff.orderFile scripts/git.orderfile + +docs/* +*.txt +configure +GNUmakefile +makefile +Makefile* +*.mak +qapi-schema*.json +qapi/*.json +include/qapi/visitor.h +include/qapi/visitor-impl.h +scripts/qapi.py +scripts/*.py +*.h +qapi/qapi-visit-core.c +*.c
When passed to git-diff (and to every other git command producing diffs and/or diffstats) with "-O" or "diff.orderFile", this list of patterns will place the more declarative / abstract hunks first, while changes to imperative code / details will be near the end of the patches. This saves on scrolling / searching and makes for easier reviewing. We intend to advise contributors in the Wiki to run git config diff.orderFile scripts/git.orderfile once, as part of their initial setup, before formatting their first (or, for repeat contributors, next) patches. See the "-O" option and the "diff.orderFile" configuration variable in git-diff(1) and git-config(1). Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Eric Blake <eblake@redhat.com> Cc: Fam Zheng <famz@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: John Snow <jsnow@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Stefan Hajnoczi <stefanha@gmail.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - "Makefile" -> "Makefile*" [Gerd] - add leading comment [Gerd] - add "docs/*" (note, there are *.txt files outside of docs/, so keeping those too) [Max, Fam, Eric] scripts/git.orderfile | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 scripts/git.orderfile -- 2.9.2