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

From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] diff: add a config option to control orderfile
Date: Tue, 24 Sep 2013 01:45:21 -0400	[thread overview]
Message-ID: <20130924054520.GC5875@sigill.intra.peff.net> (raw)
In-Reply-To: <20130923213729.GE9464@google.com>

On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:

> >> Add --order-sensitive to get historical unstable behaviour.
> 
> The --order-sensitive option seems confusing.  How do I use it to
> replicate a historical patch-id?  If I record all options that might
> have influenced ordering (which are those?) then am I guaranteed to
> get a reproducible result?  

Yes, I have the same complaint. I'd much rather the classic mode be
given a name of some sort, and then have a "--patch-id-mode=classic"
parameter (or probably some better name) that sets all of the
parameters. Then you know that two implementations using "classic"
should get the same output, and so forth if we have to tweak it again.

> So I would prefer either of the following over the above:
> 
>  a) When asked to compute the patch-id of a seekable file, use the
>     current streaming implementation until you notice a filename that
>     is out of order.  Then start over with sorted hunks (for example
>     building a table of offsets within the patch for each hunk to
>     support this).
> 
>     When asked to compute the patch-id of an unseekable file, stream
>     to a temporary file under $GIT_DIR to get a seekable file.

This would mean that everybody, whether they care about compatibility or
not, would have to pay the price to spool to $GIT_DIR, right? That's not
great, as most cases would not care.

>  b) Unconditionally use the new patch-id definition that is stable
>     under permutation of hunks.  If and when someone complains that
>     this invalidates their old patch-ids, they can work on adding a
>     nice interface for getting the old-style patch-ids.  I suspect it
>     just wouldn't come up.

I think I'd prefer this one. The "--patch-id-mode" above is how I would
do it, and in general when we are potentially breaking compatibility,
it's nice to anticipate and give an escape hatch. But I do find it
reasonably unlikely that this will come up, so maybe this is a good time
to practice YAGNI.

-Peff