On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote:
> 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.
> 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.
> 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.
> 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
> > 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?
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html