Re: [PATCH] diff: add a config option to control orderfile

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Sep 15, 2013 at 10:49:00AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 04, 2013 at 12:08:15AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Sep 03, 2013 at 10:12:18AM -0700, Junio C Hamano wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > I always want my diffs to show header files first,
> > > > then .c files, then the rest. Make it possible to
> > > > set orderfile though a config option to achieve this.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > 
> > > I admit that I was the guilty one who introduced the orderfile, but
> > > I think the feature was misguided (for one thing, it breaks the
> > > patch-id, I think).
> > 
> > This should be easy to fix - just sort before applying the ID, no?
> 
> Ping. Any comment on this idea?

Actually, after I've looked at the code,
diffcore_order is already already called for patch-id.
So patch-id shouldn't be broken by specifying the orderfile.
Why then is the feature misguided?

> > > So I am moderately hesitant to promote its use
> > > with an addition like this.
> > 
> > The status quo just makes people insist on using a script to generate patches for
> > mail.  Some projects put interface documentation in the header files, in
> > that case it makes sense to put .h before .c in the patch as it makes
> > review easier.
> > 
> > In fact I sometimes reorder chunks in the patch manually for readability -
> > it's probably worth finding a way to support this.
> > 
> > 
> > > >  diff.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/diff.c b/diff.c
> > > > index 208094f..cca0767 100644
> > > > --- a/diff.c
> > > > +++ b/diff.c
> > > > @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +        if (!strcmp(var, "diff.orderfile"))
> > > > +                return git_config_string(&default_diff_options.orderfile, var, value);
> > > > +
> > > >  	if (!prefixcmp(var, "submodule."))
> > > >  		return parse_submodule_config_option(var, value);