Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

From: Michael S. Tsirkin <mst@redhat.com>
Date: Apr 24, 2014, 06:29 UTC
Message-ID: <20140424062938.GA30231@redhat.com>
In-Reply-To: <xmqqzjjbwvk9.fsf@gitster.dls.corp.google.com>

On Wed, Apr 23, 2014 at 03:05:42PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote:
> >> Are these three patches the same as what has been queued on
> >> mt/patch-id-stable topic and cooking in 'next' for a few weeks?
> >
> > Not exactly - at your request I implemented git config
> > options to control patch id behaviour.
> > Documentation and tests updated to match.
> 
> After comparing the patches 4-6 and the one that has been in 'next'
> for a few weeks, I tried to like the new one, but I couldn't.

I'm fine with the one in next too. I was under the impression that you wanted me to change the behaviour so I worked on this, but previous version was sufficient for my purposes (which is really all about putting diff.orderfile upstream).

Show 20 quoted lines

> The new one, if I am reading the patch correctly, makes the stable
> variant the default if
> 
>  - the configuration explicitly asks to use it; or
> 
>  - diff.orderfile, which is a new configuration that did not exist,
>    is used.
> 
> At the first glance, the latter makes it look as if that will not
> hurt any existing users/repositories, but the thing is, the producer
> of the patches is different from the consumer of the patches.  There
> needs to be a way for a consumer to say "I need stable variant" on
> the patches when computing "patch-id" on a patch that arrived.  As
> long as two different producers use two different orders, the
> consumer of the patches from these two sources is forced to use the
> stable variant if some sort of cache is kept keyed with the
> patch-ids.
> 
> But "diff.orderfile" configuration is all and only about the
> producer, and does not help the case at all.

OK, we can just drop that, that's easy.

Show 10 quoted lines

> Compared to it, the previous version forced people who do not have a
> particular reason to choose between the variants to use the new
> "stable" variant, which was a lot simpler solution.
> 
> The reason why I merged the previous one to 'next' was because I
> wanted to see if the behaviour change is as grave as I thought, or I
> was just being unnecessary cautious.  If there is no third-party
> script that precomputes something and stores them under these hashes
> that breaks with this change, I do not see any reason not to make
> the new "stable" one the default.

Ah okay, so we just wait a bit and see and if all is quiet the existing patch will graduate to master with time? Totally cool. I thought you wanted me to add the config option, but if everyone is happy as is, I don't need it personally at all.

> I however suspect that the ideal "stable" implementation may be
> different from what you implemented.  What if we compute a patch-id
> on a reordered patch as if the files came in the "usual" order?

ATM patch id does not have any concept of the usual order, so that's one problem - how does one figure out what the order would be? I have no idea - is this documented anywhere? Also I'm guessing this would depend on the state of the git tree which would be another behaviour change: previously patch-id worked fine outside any git tree.

Show 6 quoted lines

>
> That would be another way to achieve the stable-ness for sane cases
> (i.e. no funny "you could split one patch with two hunks into two
> patches with one hunk each twice mentioning the same path" which is
> totally an uninteresting use case---diff.orderfile would not even
> produce such a patch)

Yes I'm thinking we should drop this hunk in the patch: let's support reordering files but not splitting them. This makes the change even smaller, so I now think we should just go for it.

> and a huge difference would be that it would
> produce the same patch-id as existing versions of Git, if the patch
> is not reordered.  Is that asking for a moon (I admit that I haven't
> looked at the code involved too deeply)?

Yes this would be a bunch of code to write - certainly much more complex than the existing small patch which just tweaks the checksum slightly to make it stable.

-- 
MST