Re: [Qemu-devel] [PATCH v2] scripts: add "git.orderfile" for ordering diff hunks by pathname patterns

[Laszlo Ersek]

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" <address@hidden>
>> Cc: Eric Blake <address@hidden>
>> Cc: Fam Zheng <address@hidden>
>> Cc: Gerd Hoffmann <address@hidden>
>> Cc: John Snow <address@hidden>
>> Cc: Max Reitz <address@hidden>
>> Cc: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>
>> 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