[Qemu-devel] a suggestion to place *.c hunks last in patches

[Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

Recent git releases support the diff.orderFile permanent setting. (In
older releases, the -O option had to be specified on the command line,
or in aliases, for the same effect, which was quite inconvenient.) From
git-diff(1):

       -O<orderfile>
           Output the patch in the order specified in the <orderfile>,
           which has one shell glob pattern per line. This overrides
           the diff.orderFile configuration variable (see git-
           config(1)). To cancel diff.orderFile, use -O/dev/null.

In my experience, an order file such as:

configure
*Makefile*
*.json
*.txt
*.h
*.c

that is, a priority order that goes from
descriptive/declarative/abstract to imperative/specific works wonders
for reviewing.

Randomly picked example:

[Qemu-devel] [PATCH] virtio-gpu: track and limit host memory allocations
http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05144.html

This patch adds several fields to several structures first, and then it
does things with those new fields. If you think about what the English
verb "to declare" means, it's clear you want to see the declaration
first (same as the compiler), and only then how the field is put to use.

Thanks!
Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Gerd Hoffmann]

On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
> Recent git releases support the diff.orderFile permanent setting.

Cool.

> configure
> *Makefile*
> *.json
> *.txt
> *.h
> *.c

I'd put *.txt to the head so doc updates come first.
Otherwise the order looks good to me.

Want sent a patch?

Can this be automatically enabled per repo, like .gitignore, so it works
without everybody tweaking its local git config?

cheers,
  Gerd

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 11:55, Gerd Hoffmann wrote:
> On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
>> Recent git releases support the diff.orderFile permanent setting.
> 
> Cool.
> 
>> configure
>> *Makefile*
>> *.json
>> *.txt
>> *.h
>> *.c
> 
> I'd put *.txt to the head so doc updates come first.

Good idea, yes.

> Otherwise the order looks good to me.
> 
> Want sent a patch?

What file for? :)

I've considered modifying
<http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
humongous already.

Nonetheless, section "Make code motion patches easy to review" mentions
some diff.* settings, so I guess a new section after it ("Format
declarative and abstract changes near the top") would be appropriate, if
there's no disagreement.

> Can this be automatically enabled per repo, like .gitignore, so it works
> without everybody tweaking its local git config?

Not to my understanding.

Thanks!
Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Fam Zheng]

On Wed, 11/30 13:03, Laszlo Ersek wrote:
> On 11/30/16 11:55, Gerd Hoffmann wrote:
> > On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
> >> Recent git releases support the diff.orderFile permanent setting.
> > 
> > Cool.
> > 
> >> configure
> >> *Makefile*
> >> *.json
> >> *.txt
> >> *.h
> >> *.c
> > 
> > I'd put *.txt to the head so doc updates come first.
> 
> Good idea, yes.
> 
> > Otherwise the order looks good to me.
> > 
> > Want sent a patch?
> 
> What file for? :)

This is a nice feature, thanks!

Does it make sense to have a .gitpublish file (for Stefan's git-publish script)
in QEMU.git? That way we can add a new profile option to git-publish and let it
build the command line accordingly. This is going to be helpful for those who
already use git-publish.

> 
> I've considered modifying
> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
> humongous already.

And this sounds good too.

Fam

> 
> Nonetheless, section "Make code motion patches easy to review" mentions
> some diff.* settings, so I guess a new section after it ("Format
> declarative and abstract changes near the top") would be appropriate, if
> there's no disagreement.
> 
> > Can this be automatically enabled per repo, like .gitignore, so it works
> > without everybody tweaking its local git config?
> 
> Not to my understanding.
> 
> Thanks!
> Laszlo
> 
> 

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Gerd Hoffmann]

  Hi,

> > Want sent a patch?
> 
> What file for? :)

create something like scripts/git.orderfile, so people have just to run
"git config diff.orderfile scripts/git.orderfile" to enable it, and we
can refine the config without having everybody update the orderfile
manually?

> I've considered modifying
> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
> humongous already.
> 
> Nonetheless, section "Make code motion patches easy to review" mentions
> some diff.* settings, so I guess a new section after it ("Format
> declarative and abstract changes near the top") would be appropriate, if
> there's no disagreement.

Yep, adding to the wiki sounds good too.

> > Can this be automatically enabled per repo, like .gitignore, so it works
> > without everybody tweaking its local git config?
> 
> Not to my understanding.

Too bad.

cheers,
  Gerd

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Michael S. Tsirkin]

On Wed, Nov 30, 2016 at 11:08:27AM +0100, Laszlo Ersek wrote:
> Recent git releases support the diff.orderFile permanent setting. (In
> older releases, the -O option had to be specified on the command line,
> or in aliases, for the same effect, which was quite inconvenient.) From
> git-diff(1):
> 
>        -O<orderfile>
>            Output the patch in the order specified in the <orderfile>,
>            which has one shell glob pattern per line. This overrides
>            the diff.orderFile configuration variable (see git-
>            config(1)). To cancel diff.orderFile, use -O/dev/null.
> 
> In my experience, an order file such as:
> 
> configure
> *Makefile*

Why add the * before Makefile? In fact, why * after it?

> *.json
> *.txt
> *.h
> *.c
> 
> that is, a priority order that goes from
> descriptive/declarative/abstract to imperative/specific works wonders
> for reviewing.
> 
> Randomly picked example:
> 
> [Qemu-devel] [PATCH] virtio-gpu: track and limit host memory allocations
> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05144.html
> 
> This patch adds several fields to several structures first, and then it
> does things with those new fields. If you think about what the English
> verb "to declare" means, it's clear you want to see the declaration
> first (same as the compiler), and only then how the field is put to use.
> 
> Thanks!
> Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 16:08, Michael S. Tsirkin wrote:
> On Wed, Nov 30, 2016 at 11:08:27AM +0100, Laszlo Ersek wrote:
>> Recent git releases support the diff.orderFile permanent setting. (In
>> older releases, the -O option had to be specified on the command line,
>> or in aliases, for the same effect, which was quite inconvenient.) From
>> git-diff(1):
>>
>>        -O<orderfile>
>>            Output the patch in the order specified in the <orderfile>,
>>            which has one shell glob pattern per line. This overrides
>>            the diff.orderFile configuration variable (see git-
>>            config(1)). To cancel diff.orderFile, use -O/dev/null.
>>
>> In my experience, an order file such as:
>>
>> configure
>> *Makefile*
> 
> Why add the * before Makefile? In fact, why * after it?

Might not be appropriate for QEMU indeed; I have that pattern because of
files in other projects. (Actually, thanks for drawing my attention to
it, because it should be *[Mm]akefile* :))

Thanks
Laszlo

>> *.json
>> *.txt
>> *.h
>> *.c
>>
>> that is, a priority order that goes from
>> descriptive/declarative/abstract to imperative/specific works wonders
>> for reviewing.
>>
>> Randomly picked example:
>>
>> [Qemu-devel] [PATCH] virtio-gpu: track and limit host memory allocations
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05144.html
>>
>> This patch adds several fields to several structures first, and then it
>> does things with those new fields. If you think about what the English
>> verb "to declare" means, it's clear you want to see the declaration
>> first (same as the compiler), and only then how the field is put to use.
>>
>> Thanks!
>> Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Stefan Hajnoczi]

On Wed, Nov 30, 2016 at 10:08 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> Recent git releases support the diff.orderFile permanent setting. (In
> older releases, the -O option had to be specified on the command line,
> or in aliases, for the same effect, which was quite inconvenient.) From
> git-diff(1):
>
>        -O<orderfile>
>            Output the patch in the order specified in the <orderfile>,
>            which has one shell glob pattern per line. This overrides
>            the diff.orderFile configuration variable (see git-
>            config(1)). To cancel diff.orderFile, use -O/dev/null.
>
> In my experience, an order file such as:
>
> configure
> *Makefile*
> *.json
> *.txt
> *.h
> *.c
>
> that is, a priority order that goes from
> descriptive/declarative/abstract to imperative/specific works wonders
> for reviewing.

Thanks, you are a gentleman and a scholar!

I've set it in my global git config.

Stefan

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 13:29, Gerd Hoffmann wrote:
>   Hi,
> 
>>> Want sent a patch?
>>
>> What file for? :)
> 
> create something like scripts/git.orderfile, so people have just to run
> "git config diff.orderfile scripts/git.orderfile" to enable it, and we
> can refine the config without having everybody update the orderfile
> manually?

Good idea -- I didn't expect this to work, but it actually does.

The -O command line option interprets a relative pathname relative to
the working directory. However, if you place a relative pathname in
diff.orderFile in .git/config, then git will look for the order file
relative to the project root directory, not relative to your working
directory (which could be any subdirectory of the project root directory).

> 
>> I've considered modifying
>> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
>> humongous already.
>>
>> Nonetheless, section "Make code motion patches easy to review" mentions
>> some diff.* settings, so I guess a new section after it ("Format
>> declarative and abstract changes near the top") would be appropriate, if
>> there's no disagreement.
> 
> Yep, adding to the wiki sounds good too.
> 
>>> Can this be automatically enabled per repo, like .gitignore, so it works
>>> without everybody tweaking its local git config?
>>
>> Not to my understanding.
> 
> Too bad.

I think recommending a simple git config command in the wiki, like you
write above, should be fine.

I guess I'll add this item (the patch and the wiki update) to my todo
list... :)

Thanks
Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On 11/30/2016 04:08 AM, Laszlo Ersek wrote:
> Recent git releases support the diff.orderFile permanent setting. (In
> older releases, the -O option had to be specified on the command line,
> or in aliases, for the same effect, which was quite inconvenient.) From
> git-diff(1):
> 
>        -O<orderfile>
>            Output the patch in the order specified in the <orderfile>,
>            which has one shell glob pattern per line. This overrides
>            the diff.orderFile configuration variable (see git-
>            config(1)). To cancel diff.orderFile, use -O/dev/null.
> 
> In my experience, an order file such as:
> 
> configure
> *Makefile*
> *.json
> *.txt
> *.h
> *.c
> 
> that is, a priority order that goes from
> descriptive/declarative/abstract to imperative/specific works wonders
> for reviewing.

I've been doing this for some time now, although I sometimes tweak the
order on a per-series basis to highlight what I think is most
interesting in that series.  My current order file contents are:

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

stemming from a patch series that touched visitor interfaces.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Michael S. Tsirkin]

On Wed, Nov 30, 2016 at 04:26:14PM +0100, Laszlo Ersek wrote:
> On 11/30/16 16:08, Michael S. Tsirkin wrote:
> > On Wed, Nov 30, 2016 at 11:08:27AM +0100, Laszlo Ersek wrote:
> >> Recent git releases support the diff.orderFile permanent setting. (In
> >> older releases, the -O option had to be specified on the command line,
> >> or in aliases, for the same effect, which was quite inconvenient.) From
> >> git-diff(1):
> >>
> >>        -O<orderfile>
> >>            Output the patch in the order specified in the <orderfile>,
> >>            which has one shell glob pattern per line. This overrides
> >>            the diff.orderFile configuration variable (see git-
> >>            config(1)). To cancel diff.orderFile, use -O/dev/null.
> >>
> >> In my experience, an order file such as:
> >>
> >> configure
> >> *Makefile*
> > 
> > Why add the * before Makefile? In fact, why * after it?
> 
> Might not be appropriate for QEMU indeed; I have that pattern because of
> files in other projects. (Actually, thanks for drawing my attention to
> it, because it should be *[Mm]akefile* :))
> 
> Thanks
> Laszlo

GNU make tries the following names, in order: 'GNUmakefile', 'makefile' and 'Makefile'.
So I would make it just that:

GNUmakefile
makefile
Makefile

but we have helpers in .mak files so add

*.mak


> >> *.json
> >> *.txt
> >> *.h
> >> *.c
> >>
> >> that is, a priority order that goes from
> >> descriptive/declarative/abstract to imperative/specific works wonders
> >> for reviewing.
> >>
> >> Randomly picked example:
> >>
> >> [Qemu-devel] [PATCH] virtio-gpu: track and limit host memory allocations
> >> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05144.html
> >>
> >> This patch adds several fields to several structures first, and then it
> >> does things with those new fields. If you think about what the English
> >> verb "to declare" means, it's clear you want to see the declaration
> >> first (same as the compiler), and only then how the field is put to use.
> >>
> >> Thanks!
> >> Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 16:41, Eric Blake wrote:
> On 11/30/2016 04:08 AM, Laszlo Ersek wrote:
>> Recent git releases support the diff.orderFile permanent setting. (In
>> older releases, the -O option had to be specified on the command line,
>> or in aliases, for the same effect, which was quite inconvenient.) From
>> git-diff(1):
>>
>>        -O<orderfile>
>>            Output the patch in the order specified in the <orderfile>,
>>            which has one shell glob pattern per line. This overrides
>>            the diff.orderFile configuration variable (see git-
>>            config(1)). To cancel diff.orderFile, use -O/dev/null.
>>
>> In my experience, an order file such as:
>>
>> configure
>> *Makefile*
>> *.json
>> *.txt
>> *.h
>> *.c
>>
>> that is, a priority order that goes from
>> descriptive/declarative/abstract to imperative/specific works wonders
>> for reviewing.
> 
> I've been doing this for some time now, although I sometimes tweak the
> order on a per-series basis to highlight what I think is most
> interesting in that series.  My current order file contents are:
> 
> 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
> 
> stemming from a patch series that touched visitor interfaces.
> 

It should also be noted -- I forgot before, sorry -- that the diff order
affects any and all diffstats as well. This is usually welcome for
individual patches (the diffstat should reflect the order of hunks in
the patch), but quite unhelpful for the cumulative diffstat across a
series (in the blurb).

I cope with this by selecting the cumulative diffstat lines in my
editor, when editing the blurb, and filtering that block through "sort".
Most of the time it works fine, without needing further touch-ups.

When the series copies or moves files (with the appropriate copy/move
detection enabled), then the cumulative diffstat (too) contains those
funky { and } characters. They don't play too nice with "sort", hence
manual reordering could be necessary in this case, for a review
experience de luxe.

Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[John Snow]

On 11/30/2016 07:03 AM, Laszlo Ersek wrote:
> On 11/30/16 11:55, Gerd Hoffmann wrote:
>> On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
>>> Recent git releases support the diff.orderFile permanent setting.
>>
>> Cool.
>>
>>> configure
>>> *Makefile*
>>> *.json
>>> *.txt
>>> *.h
>>> *.c
>>
>> I'd put *.txt to the head so doc updates come first.
> 
> Good idea, yes.
> 
>> Otherwise the order looks good to me.
>>
>> Want sent a patch?
> 
> What file for? :)
> 
> I've considered modifying
> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
> humongous already.
> 

<OFFTOPIC>

I am itching to split this article up.

It was a giant monolith when I found it, so I split it up into "phases"
and added a TOC, but it's still too unapproachable.

I'm thinking _two_ articles:

(1) A quick and dirty checklist that people can reference for their
first submissions to make sure they are in the correct universe when
submitting a patch, written expressly to be easy to digest without being
overwhelming, and

(2) A more detailed article we can reference when offering specific
feedback to newer contributors who did mostly everything correct.

</OFFTOPIC>

> Nonetheless, section "Make code motion patches easy to review" mentions
> some diff.* settings, so I guess a new section after it ("Format
> declarative and abstract changes near the top") would be appropriate, if
> there's no disagreement.
> 
>> Can this be automatically enabled per repo, like .gitignore, so it works
>> without everybody tweaking its local git config?
> 
> Not to my understanding.
> 
> Thanks!
> Laszlo
> 
> 

Great suggestion. implementing immediately. :)

-- 
—js

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 21:48, John Snow wrote:
> 
> 
> On 11/30/2016 07:03 AM, Laszlo Ersek wrote:
>> On 11/30/16 11:55, Gerd Hoffmann wrote:
>>> On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
>>>> Recent git releases support the diff.orderFile permanent setting.
>>>
>>> Cool.
>>>
>>>> configure
>>>> *Makefile*
>>>> *.json
>>>> *.txt
>>>> *.h
>>>> *.c
>>>
>>> I'd put *.txt to the head so doc updates come first.
>>
>> Good idea, yes.
>>
>>> Otherwise the order looks good to me.
>>>
>>> Want sent a patch?
>>
>> What file for? :)
>>
>> I've considered modifying
>> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
>> humongous already.
>>
> 
> <OFFTOPIC>
> 
> I am itching to split this article up.
> 
> It was a giant monolith when I found it, so I split it up into "phases"
> and added a TOC, but it's still too unapproachable.

So is it large, in your opinion? No, it's not large. (I know I wrote
"humongous", but that was sort of tongue in cheeck.) This is large:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

:)

(Note that it describes the -O option too -- at the time of writing,
diff.orderFile didn't exist yet. I guess I should tack another update to
my todo list, but I can't see its end from here.)

> I'm thinking _two_ articles:
> 
> (1) A quick and dirty checklist that people can reference for their
> first submissions to make sure they are in the correct universe when
> submitting a patch, written expressly to be easy to digest without being
> overwhelming, and
> 
> (2) A more detailed article we can reference when offering specific
> feedback to newer contributors who did mostly everything correct.
> 
> </OFFTOPIC>

That's very kind to new contributors!

> 
>> Nonetheless, section "Make code motion patches easy to review" mentions
>> some diff.* settings, so I guess a new section after it ("Format
>> declarative and abstract changes near the top") would be appropriate, if
>> there's no disagreement.
>>
>>> Can this be automatically enabled per repo, like .gitignore, so it works
>>> without everybody tweaking its local git config?
>>
>> Not to my understanding.
>>
>> Thanks!
>> Laszlo
>>
>>
> 
> Great suggestion. implementing immediately. :)

"Implementing" as in you're going to submit the order file patch, and
update the wiki? ;)

(Just kidding, I can do those things. Later.)

Thanks!
Laszlo

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Laszlo Ersek]

On 11/30/16 13:27, Fam Zheng wrote:
> On Wed, 11/30 13:03, Laszlo Ersek wrote:
>> On 11/30/16 11:55, Gerd Hoffmann wrote:
>>> On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
>>>> Recent git releases support the diff.orderFile permanent setting.
>>>
>>> Cool.
>>>
>>>> configure
>>>> *Makefile*
>>>> *.json
>>>> *.txt
>>>> *.h
>>>> *.c
>>>
>>> I'd put *.txt to the head so doc updates come first.
>>
>> Good idea, yes.
>>
>>> Otherwise the order looks good to me.
>>>
>>> Want sent a patch?
>>
>> What file for? :)
> 
> This is a nice feature, thanks!
> 
> Does it make sense to have a .gitpublish file (for Stefan's git-publish script)
> in QEMU.git?

Sorry, I didn't mean to ignore this -- I just don't use git-publish, so
I can't comment.

Thanks
Laszlo

> That way we can add a new profile option to git-publish and let it
> build the command line accordingly. This is going to be helpful for those who
> already use git-publish.
> 
>>
>> I've considered modifying
>> <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
>> humongous already.
> 
> And this sounds good too.
> 
> Fam
> 
>>
>> Nonetheless, section "Make code motion patches easy to review" mentions
>> some diff.* settings, so I guess a new section after it ("Format
>> declarative and abstract changes near the top") would be appropriate, if
>> there's no disagreement.
>>
>>> Can this be automatically enabled per repo, like .gitignore, so it works
>>> without everybody tweaking its local git config?
>>
>> Not to my understanding.
>>
>> Thanks!
>> Laszlo
>>
>>

Re: [Qemu-devel] a suggestion to place *.c hunks last in patches

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Wed, Nov 30, 2016 at 03:48:01PM -0500, John Snow wrote:
> On 11/30/2016 07:03 AM, Laszlo Ersek wrote:
> > On 11/30/16 11:55, Gerd Hoffmann wrote:
> >> On Mi, 2016-11-30 at 11:08 +0100, Laszlo Ersek wrote:
> >>> Recent git releases support the diff.orderFile permanent setting.
> >>
> >> Cool.
> >>
> >>> configure
> >>> *Makefile*
> >>> *.json
> >>> *.txt
> >>> *.h
> >>> *.c
> >>
> >> I'd put *.txt to the head so doc updates come first.
> > 
> > Good idea, yes.
> > 
> >> Otherwise the order looks good to me.
> >>
> >> Want sent a patch?
> > 
> > What file for? :)
> > 
> > I've considered modifying
> > <http://wiki.qemu.org/Contribute/SubmitAPatch>, but that article is
> > humongous already.
> > 
> 
> <OFFTOPIC>
> 
> I am itching to split this article up.
> 
> It was a giant monolith when I found it, so I split it up into "phases"
> and added a TOC, but it's still too unapproachable.
> 
> I'm thinking _two_ articles:
> 
> (1) A quick and dirty checklist that people can reference for their
> first submissions to make sure they are in the correct universe when
> submitting a patch, written expressly to be easy to digest without being
> overwhelming, and

Sounds good.

Another option for making the first time contribution process easier is
to automate as many of the rules as possible.  Can some of it be
implemented in checkpatch.pl?  Are there git-hooks we should supply?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]