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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

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

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