Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering
"Michael S. Tsirkin" writes: > On Wed, Apr 23, 2014 at 03:05:42PM -0700, Junio C Hamano wrote: > >> After comparing the patches 4-6 and the one that has been in 'next' >> for a few weeks, I tried to like the new one, but I couldn't. > > I'm fine with the one in next too. > I was under the impression that you wanted me to change > the behaviour so I worked on this,... What I wanted to see was to make sure this would not kick in unless the user explicitly asks. "patchid.stable" configuration variable is very much welcome, and if it defaulted to false with or without "diff.orderfile", that would have been very much welcome. Then nobody will be surprised. >> But "diff.orderfile" configuration is all and only about the >> producer, and does not help the case at all. > > OK, we can just drop that, that's easy. > >> Compared to it, the previous version forced people who do not have a >> particular reason to choose between the variants to use the new >> "stable" variant, which was a lot simpler solution. >> >> The reason why I merged the previous one to 'next' was because I >> wanted to see if the behaviour change is as grave as I thought, or I >> was just being unnecessary cautious. If there is no third-party >> script that precomputes something and stores them under these hashes >> that breaks with this change, I do not see any reason not to make >> the new "stable" one the default. > > Ah okay, so we just wait a bit and see and if all is quiet the > existing patch will graduate to master with time? > Totally cool. > I thought you wanted me to add the config option, but if everyone > is happy as is, I don't need it personally at all. ... or if we see problems, then build a fix on top to introduce "patchid.stable" that defaults to false and not linking the feature with "diff.ordefile". Adding a new feature turned-off by default is the safer thing to do. When nothing changes, by defintion there won't be a new breakage. That is the default stance this project has taken for a long time, and what made us trusted by projects that manage their source files using our product. It however is to favour stability and "no surprise" over progress, and it may not be an optimal thing to do if the new feature is compellingly good. In some cases like this one, we may need to experiment to find out the need of the users, and introducing the feature with a configuration that is explicitly opt-in is one way to do so. We may or may not see many people using that feature without disrupting existing users that way. Cooking in 'next' with the feature turned-on by default is another way that is riskier, but in this particular case, the population that is likely to be affected are power users, which would match the audience of 'next'---those who still want stability but want to be closer to the cutting edge. -- 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
Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering
On Wed, Apr 23, 2014 at 03:05:42PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote: > >> Are these three patches the same as what has been queued on > >> mt/patch-id-stable topic and cooking in 'next' for a few weeks? > > > > Not exactly - at your request I implemented git config > > options to control patch id behaviour. > > Documentation and tests updated to match. > > After comparing the patches 4-6 and the one that has been in 'next' > for a few weeks, I tried to like the new one, but I couldn't. I'm fine with the one in next too. I was under the impression that you wanted me to change the behaviour so I worked on this, but previous version was sufficient for my purposes (which is really all about putting diff.orderfile upstream). > The new one, if I am reading the patch correctly, makes the stable > variant the default if > > - the configuration explicitly asks to use it; or > > - diff.orderfile, which is a new configuration that did not exist, >is used. > > At the first glance, the latter makes it look as if that will not > hurt any existing users/repositories, but the thing is, the producer > of the patches is different from the consumer of the patches. There > needs to be a way for a consumer to say "I need stable variant" on > the patches when computing "patch-id" on a patch that arrived. As > long as two different producers use two different orders, the > consumer of the patches from these two sources is forced to use the > stable variant if some sort of cache is kept keyed with the > patch-ids. > > But "diff.orderfile" configuration is all and only about the > producer, and does not help the case at all. OK, we can just drop that, that's easy. > Compared to it, the previous version forced people who do not have a > particular reason to choose between the variants to use the new > "stable" variant, which was a lot simpler solution. > > The reason why I merged the previous one to 'next' was because I > wanted to see if the behaviour change is as grave as I thought, or I > was just being unnecessary cautious. If there is no third-party > script that precomputes something and stores them under these hashes > that breaks with this change, I do not see any reason not to make > the new "stable" one the default. Ah okay, so we just wait a bit and see and if all is quiet the existing patch will graduate to master with time? Totally cool. I thought you wanted me to add the config option, but if everyone is happy as is, I don't need it personally at all. > I however suspect that the ideal "stable" implementation may be > different from what you implemented. What if we compute a patch-id > on a reordered patch as if the files came in the "usual" order? ATM patch id does not have any concept of the usual order, so that's one problem - how does one figure out what the order would be? I have no idea - is this documented anywhere? Also I'm guessing this would depend on the state of the git tree which would be another behaviour change: previously patch-id worked fine outside any git tree. > > That would be another way to achieve the stable-ness for sane cases > (i.e. no funny "you could split one patch with two hunks into two > patches with one hunk each twice mentioning the same path" which is > totally an uninteresting use case---diff.orderfile would not even > produce such a patch) Yes I'm thinking we should drop this hunk in the patch: let's support reordering files but not splitting them. This makes the change even smaller, so I now think we should just go for it. > and a huge difference would be that it would > produce the same patch-id as existing versions of Git, if the patch > is not reordered. Is that asking for a moon (I admit that I haven't > looked at the code involved too deeply)? Yes this would be a bunch of code to write - certainly much more complex than the existing small patch which just tweaks the checksum slightly to make it stable. -- MST -- 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
Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering
"Michael S. Tsirkin" writes: > On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote: >> Are these three patches the same as what has been queued on >> mt/patch-id-stable topic and cooking in 'next' for a few weeks? > > Not exactly - at your request I implemented git config > options to control patch id behaviour. > Documentation and tests updated to match. After comparing the patches 4-6 and the one that has been in 'next' for a few weeks, I tried to like the new one, but I couldn't. The new one, if I am reading the patch correctly, makes the stable variant the default if - the configuration explicitly asks to use it; or - diff.orderfile, which is a new configuration that did not exist, is used. At the first glance, the latter makes it look as if that will not hurt any existing users/repositories, but the thing is, the producer of the patches is different from the consumer of the patches. There needs to be a way for a consumer to say "I need stable variant" on the patches when computing "patch-id" on a patch that arrived. As long as two different producers use two different orders, the consumer of the patches from these two sources is forced to use the stable variant if some sort of cache is kept keyed with the patch-ids. But "diff.orderfile" configuration is all and only about the producer, and does not help the case at all. Compared to it, the previous version forced people who do not have a particular reason to choose between the variants to use the new "stable" variant, which was a lot simpler solution. The reason why I merged the previous one to 'next' was because I wanted to see if the behaviour change is as grave as I thought, or I was just being unnecessary cautious. If there is no third-party script that precomputes something and stores them under these hashes that breaks with this change, I do not see any reason not to make the new "stable" one the default. I however suspect that the ideal "stable" implementation may be different from what you implemented. What if we compute a patch-id on a reordered patch as if the files came in the "usual" order? That would be another way to achieve the stable-ness for sane cases (i.e. no funny "you could split one patch with two hunks into two patches with one hunk each twice mentioning the same path" which is totally an uninteresting use case---diff.orderfile would not even produce such a patch) and a huge difference would be that it would produce the same patch-id as existing versions of Git, if the patch is not reordered. Is that asking for a moon (I admit that I haven't looked at the code involved too deeply)? -- 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
Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering
On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote: > Are these three patches the same as what has been queued on > mt/patch-id-stable topic and cooking in 'next' for a few weeks? Not exactly - at your request I implemented git config options to control patch id behaviour. Documentation and tests updated to match. -- 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
Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering
Are these three patches the same as what has been queued on mt/patch-id-stable topic and cooking in 'next' for a few weeks? -- 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
