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

From: Michael S. Tsirkin <mst@redhat.com>
Date: Apr 24, 2014, 21:32 UTC
Message-ID: <20140424213238.GB9129@redhat.com>
In-Reply-To: <20140424173043.GJ15516@google.com>

On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote:

Show 31 quoted lines

> Hi,
> 
> Michael S. Tsirkin wrote:
> 
> > Patch id changes if users
> > 1. reorder file diffs that make up a patch
> > or
> > 2. split a patch up to multiple diffs that touch the same path
> > (keeping hunks within a single diff ordered to make patch valid).
> >
> > As the result is functionally equivalent, a different patch id is
> > surprising to many users.
> 
> Hm.
> 
> If the goal is that functionally equivalent patches are guaranteed to
> produce the same patch-id, I wonder if we should be doing something
> like the following:
> 
>  1. apply the patch in memory
>  2. generate a new diff
>  3. use that new diff to produce a patch-id
> 
> Otherwise issues like --diff-algorithm=patience versus =myers will
> create trouble too.  I don't think that avoiding false negatives for
> patch comparison without doing something like that is really possible.
> 
> On the other hand if someone reorders file diffs within a patch, that
> is a potentially very common thing to do and something worth fixing.
> In other words, while your (1) makes perfect sense to me, case (2)
> seems less convincing.

I agree it's less convincing: one would have to edit patch by hand (which I used to sometimes do to make important parts more prominent, but stopped doing in favor of splitting a patch). I'm not 100% sure whether it's worth supporting or not.

Show 5 quoted lines

> The downside of allowing reordering hunks is that it can potentially
> make different patches to be treated the same (for example if they
> were making similar changes to different functions) when the ordering
> previously caused them to be distinguished.  But that wasn't something
> people could count on anyway, so I don't mind.

I think this example convinces me. I'll drop this support in the next version.

Show 23 quoted lines

> Should the internal patch-id computation used by commands like 'git
> cherry' (see diff.c::diff_get_patch_id) get the same change?  (Not a
> rhetorical question --- I don't know what the right choice would be
> there.)
> 
> [...]
> > The new behaviour is enabled
> > - when patchid.stable is true
> > - when --stable flag is present
> >
> > Using a new flag --unstable or setting patchid.stable to false force
> > the historical behaviour.
> 
> Which is the default?
> 
> [...]
> >  builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 73 insertions(+), 16 deletions(-)
> 
> Documentation?  Tests?
> 
> Thanks,
> Jonathan