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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 24, 2014, 17:30 UTC
Message-ID: <20140424173043.GJ15516@google.com>
In-Reply-To: <1398331809-11309-4-git-send-email-mst@redhat.com>

Hi,

Michael S. Tsirkin wrote:

Show 8 quoted lines

> 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.

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.

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.)

[...]

Show 6 quoted lines

> 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