On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote: > 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. > 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 > 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 -- 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
