{"thread":{"id":"34819","subject":"[PATCH] diff: add a config option to control orderfile","startedAt":"2013-08-31T19:44:25Z","lastAt":"2013-09-24T22:08:49Z","messageCount":32,"participants":["Michael S. Tsirkin","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"226446","messageId":"20130831194425.GA14706@redhat.com","threadId":"34819","inReplyTo":null,"subject":"[PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-08-31T19:44:25Z","receivedAt":"2013-08-31T19:44:25Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"I always want my diffs to show header files first,\nthen .c files, then the rest. Make it possible to\nset orderfile though a config option to achieve this.\n\nSigned-off-by: Michael S. Tsirkin \n---\n diff.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 208094f..cca0767 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+ if (!strcmp(var, \"diff.orderfile\"))\n+ return git_config_string(&default_diff_options.orderfile, var, value);\n+\n \tif (!prefixcmp(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n-- \nMST\n"},{"id":"226637","messageId":"xmqqioyhu8wd.fsf@gitster.dls.corp.google.com","threadId":"34819","inReplyTo":"20130831194425.GA14706@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T17:12:18Z","receivedAt":"2013-09-03T17:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> I always want my diffs to show header files first,\n> then .c files, then the rest. Make it possible to\n> set orderfile though a config option to achieve this.\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n\nI admit that I was the guilty one who introduced the orderfile, but\nI think the feature was misguided (for one thing, it breaks the\npatch-id, I think). So I am moderately hesitant to promote its use\nwith an addition like this.\n\n> diff.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 208094f..cca0767 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> + if (!strcmp(var, \"diff.orderfile\"))\n> + return git_config_string(&default_diff_options.orderfile, var, value);\n> +\n> \tif (!prefixcmp(var, \"submodule.\"))\n> \t\treturn parse_submodule_config_option(var, value);\n"},{"id":"226676","messageId":"20130903210815.GB24480@redhat.com","threadId":"34819","inReplyTo":"xmqqioyhu8wd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-03T21:08:15Z","receivedAt":"2013-09-03T21:08:15Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 03, 2013 at 10:12:18AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > I always want my diffs to show header files first,\n> > then .c files, then the rest. Make it possible to\n> > set orderfile though a config option to achieve this.\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> > ---\n> \n> I admit that I was the guilty one who introduced the orderfile, but\n> I think the feature was misguided (for one thing, it breaks the\n> patch-id, I think).\n\nThis should be easy to fix - just sort before applying the ID, no?\n\n> So I am moderately hesitant to promote its use\n> with an addition like this.\n\nThe status quo just makes people insist on using a script to generate patches for\nmail. Some projects put interface documentation in the header files, in\nthat case it makes sense to put .h before .c in the patch as it makes\nreview easier.\n\nIn fact I sometimes reorder chunks in the patch manually for readability -\nit's probably worth finding a way to support this.\n\n\n> > diff.c | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 208094f..cca0767 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> > \t\treturn 0;\n> > \t}\n> > \n> > + if (!strcmp(var, \"diff.orderfile\"))\n> > + return git_config_string(&default_diff_options.orderfile, var, value);\n> > +\n> > \tif (!prefixcmp(var, \"submodule.\"))\n> > \t\treturn parse_submodule_config_option(var, value);\n"},{"id":"227689","messageId":"20130915074900.GA30438@redhat.com","threadId":"34819","inReplyTo":"20130903210815.GB24480@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-15T07:49:00Z","receivedAt":"2013-09-15T07:49:00Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Wed, Sep 04, 2013 at 12:08:15AM +0300, Michael S. Tsirkin wrote:\n> On Tue, Sep 03, 2013 at 10:12:18AM -0700, Junio C Hamano wrote:\n> > \"Michael S. Tsirkin\" writes:\n> > \n> > > I always want my diffs to show header files first,\n> > > then .c files, then the rest. Make it possible to\n> > > set orderfile though a config option to achieve this.\n> > >\n> > > Signed-off-by: Michael S. Tsirkin \n> > > ---\n> > \n> > I admit that I was the guilty one who introduced the orderfile, but\n> > I think the feature was misguided (for one thing, it breaks the\n> > patch-id, I think).\n> \n> This should be easy to fix - just sort before applying the ID, no?\n\nPing. Any comment on this idea?\n\n> > So I am moderately hesitant to promote its use\n> > with an addition like this.\n> \n> The status quo just makes people insist on using a script to generate patches for\n> mail. Some projects put interface documentation in the header files, in\n> that case it makes sense to put .h before .c in the patch as it makes\n> review easier.\n> \n> In fact I sometimes reorder chunks in the patch manually for readability -\n> it's probably worth finding a way to support this.\n> \n> \n> > > diff.c | 3 +++\n> > > 1 file changed, 3 insertions(+)\n> > >\n> > > diff --git a/diff.c b/diff.c\n> > > index 208094f..cca0767 100644\n> > > --- a/diff.c\n> > > +++ b/diff.c\n> > > @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> > > \t\treturn 0;\n> > > \t}\n> > > \n> > > + if (!strcmp(var, \"diff.orderfile\"))\n> > > + return git_config_string(&default_diff_options.orderfile, var, value);\n> > > +\n> > > \tif (!prefixcmp(var, \"submodule.\"))\n> > > \t\treturn parse_submodule_config_option(var, value);\n"},{"id":"227690","messageId":"20130915080830.GA30465@redhat.com","threadId":"34819","inReplyTo":"20130915074900.GA30438@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-15T08:08:30Z","receivedAt":"2013-09-15T08:08:30Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Sun, Sep 15, 2013 at 10:49:00AM +0300, Michael S. Tsirkin wrote:\n> On Wed, Sep 04, 2013 at 12:08:15AM +0300, Michael S. Tsirkin wrote:\n> > On Tue, Sep 03, 2013 at 10:12:18AM -0700, Junio C Hamano wrote:\n> > > \"Michael S. Tsirkin\" writes:\n> > > \n> > > > I always want my diffs to show header files first,\n> > > > then .c files, then the rest. Make it possible to\n> > > > set orderfile though a config option to achieve this.\n> > > >\n> > > > Signed-off-by: Michael S. Tsirkin \n> > > > ---\n> > > \n> > > I admit that I was the guilty one who introduced the orderfile, but\n> > > I think the feature was misguided (for one thing, it breaks the\n> > > patch-id, I think).\n> > \n> > This should be easy to fix - just sort before applying the ID, no?\n> \n> Ping. Any comment on this idea?\n\nActually, after I've looked at the code,\ndiffcore_order is already already called for patch-id.\nSo patch-id shouldn't be broken by specifying the orderfile.\nWhy then is the feature misguided?\n\n> > > So I am moderately hesitant to promote its use\n> > > with an addition like this.\n> > \n> > The status quo just makes people insist on using a script to generate patches for\n> > mail. Some projects put interface documentation in the header files, in\n> > that case it makes sense to put .h before .c in the patch as it makes\n> > review easier.\n> > \n> > In fact I sometimes reorder chunks in the patch manually for readability -\n> > it's probably worth finding a way to support this.\n> > \n> > \n> > > > diff.c | 3 +++\n> > > > 1 file changed, 3 insertions(+)\n> > > >\n> > > > diff --git a/diff.c b/diff.c\n> > > > index 208094f..cca0767 100644\n> > > > --- a/diff.c\n> > > > +++ b/diff.c\n> > > > @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> > > > \t\treturn 0;\n> > > > \t}\n> > > > \n> > > > + if (!strcmp(var, \"diff.orderfile\"))\n> > > > + return git_config_string(&default_diff_options.orderfile, var, value);\n> > > > +\n> > > > \tif (!prefixcmp(var, \"submodule.\"))\n> > > > \t\treturn parse_submodule_config_option(var, value);\n"},{"id":"227756","messageId":"xmqq8uyv1kka.fsf@gitster.dls.corp.google.com","threadId":"34819","inReplyTo":"20130915080830.GA30465@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T16:26:13Z","receivedAt":"2013-09-17T16:26:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> Actually, after I've looked at the code,\n> diffcore_order is already already called for patch-id.\n\nThat was a band-aid for an ill-thought-out orderfile misfeature to\nhide the breakage. You somehow make sure that you pass the same\norderfile to diff generating machinery used internal to patch-id\ngeneration. The standalone \"git patch-id\" would be reading the\npatch in the order you give it when you are feeding a patch somebody\nsent you via the mailing list using an order unknown to you, no?\n\nBefore making it _easier_ to use the orderfile to generate diffs, we\nwould need to prepare the parts that will be broken by wider use of\nthe feature.\n"},{"id":"227758","messageId":"20130917164226.GB20672@redhat.com","threadId":"34819","inReplyTo":"xmqq8uyv1kka.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T16:42:26Z","receivedAt":"2013-09-17T16:42:26Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 09:26:13AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > Actually, after I've looked at the code,\n> > diffcore_order is already already called for patch-id.\n> \n> That was a band-aid for an ill-thought-out orderfile misfeature to\n> hide the breakage. You somehow make sure that you pass the same\n> orderfile to diff generating machinery used internal to patch-id\n> generation. The standalone \"git patch-id\" would be reading the\n> patch in the order you give it when you are feeding a patch somebody\n> sent you via the mailing list using an order unknown to you, no?\n\nYes but that's already the case, right?\nPeople don't have to use git to generate patches.\n\nSo might it not be useful to tweak patch id to\nsort the diff, making it a bit more stable?\nThis means it needs to pass twice over the file, but is this\na huge issue?\npass 1 - get offsets of chunks and sort them\npass 2 - seek each chunk and hash\n\nwhat do you think?\n\n> Before making it _easier_ to use the orderfile to generate diffs, we\n> would need to prepare the parts that will be broken by wider use of\n> the feature.\n\nI'll be glad to help do this if you tell me what these parts are.\nanything else besides fixing besides the stand-alone patch id?\n\n-- \nMST\n"},{"id":"227763","messageId":"xmqqfvt3z7i4.fsf@gitster.dls.corp.google.com","threadId":"34819","inReplyTo":"20130917164226.GB20672@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T17:24:19Z","receivedAt":"2013-09-17T17:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> So might it not be useful to tweak patch id to\n> sort the diff, making it a bit more stable?\n\nThat is one thing that needs to be done, I think. But it would be\nunfortunate if we have to do that unconditionally, though, as we may\nbe \"buffering\" many hundred kilobytes of patch text in core. If we\ncan do so without regressing the streaming performance for the most\ncommon case of not using the orderfile on the generating side (hence\nnot having to sort on the receiving end), it would be ideal. I am\nnot sure offhand how much code damage we are talking about, though.\n\n> I'll be glad to help do this if you tell me what these parts are.\n> anything else besides fixing besides the stand-alone patch id?\n\nOff the top of my head I do not think of one (but that is not a\nguarantee that there isn't any).\n"},{"id":"227764","messageId":"20130917172829.GA21121@redhat.com","threadId":"34819","inReplyTo":"xmqqfvt3z7i4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T17:28:30Z","receivedAt":"2013-09-17T17:28:30Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > So might it not be useful to tweak patch id to\n> > sort the diff, making it a bit more stable?\n> \n> That is one thing that needs to be done, I think. But it would be\n> unfortunate if we have to do that unconditionally, though, as we may\n> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> can do so without regressing the streaming performance for the most\n> common case of not using the orderfile on the generating side (hence\n> not having to sort on the receiving end), it would be ideal. I am\n> not sure offhand how much code damage we are talking about, though.\n\nSo make it conditional on the presence of the orderefile option?\n\n> > I'll be glad to help do this if you tell me what these parts are.\n> > anything else besides fixing besides the stand-alone patch id?\n> \n> Off the top of my head I do not think of one (but that is not a\n> guarantee that there isn't any).\n"},{"id":"227769","messageId":"xmqq38p3z5kg.fsf@gitster.dls.corp.google.com","threadId":"34819","inReplyTo":"20130917172829.GA21121@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T18:06:07Z","receivedAt":"2013-09-17T18:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n>> \"Michael S. Tsirkin\" writes:\n>> \n>> > So might it not be useful to tweak patch id to\n>> > sort the diff, making it a bit more stable?\n>> \n>> That is one thing that needs to be done, I think. But it would be\n>> unfortunate if we have to do that unconditionally, though, as we may\n>> be \"buffering\" many hundred kilobytes of patch text in core. If we\n>> can do so without regressing the streaming performance for the most\n>> common case of not using the orderfile on the generating side (hence\n>> not having to sort on the receiving end), it would be ideal. I am\n>> not sure offhand how much code damage we are talking about, though.\n>\n> So make it conditional on the presence of the orderefile option?\n\nThat would mean that those who set orderfile from configuration in\nthe future will have to always suffer, I would think. Is that\nacceptable? I dunno.\n\nAlso, if the sender used a non-standard order, the recipient does\nnot know what order the patch was generated, and the recipient does\nnot use a custom orderfile, what should happen? I thought your idea\nwas to normalize by using some canonical order that is not affected\nby the orderfile to make sure patch-id stays stable, so I would\nimagine that such a recipient who does not have orderfile specified\nstill needs to sort before hashing, no?\n"},{"id":"227777","messageId":"20130917192532.GA21419@redhat.com","threadId":"34819","inReplyTo":"xmqq38p3z5kg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T19:25:32Z","receivedAt":"2013-09-17T19:25:32Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 11:06:07AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> >> \"Michael S. Tsirkin\" writes:\n> >> \n> >> > So might it not be useful to tweak patch id to\n> >> > sort the diff, making it a bit more stable?\n> >> \n> >> That is one thing that needs to be done, I think. But it would be\n> >> unfortunate if we have to do that unconditionally, though, as we may\n> >> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> >> can do so without regressing the streaming performance for the most\n> >> common case of not using the orderfile on the generating side (hence\n> >> not having to sort on the receiving end), it would be ideal. I am\n> >> not sure offhand how much code damage we are talking about, though.\n> >\n> > So make it conditional on the presence of the orderefile option?\n> \n> That would mean that those who set orderfile from configuration in\n> the future will have to always suffer, I would think. Is that\n> acceptable? I dunno.\n\nWell it's just two passes over a diff. Are we optimizing this\nfor tape based systems or something?\n\n> Also, if the sender used a non-standard order, the recipient does\n> not know what order the patch was generated, and the recipient does\n> not use a custom orderfile, what should happen? I thought your idea\n> was to normalize by using some canonical order that is not affected\n> by the orderfile to make sure patch-id stays stable, so I would\n> imagine that such a recipient who does not have orderfile specified\n> still needs to sort before hashing, no?\n\nIf you like, but I don't really care. It's fine with me to make this\nconditional on specifying an order file on recepient side.\nOr we can make it a separate option.\nOr we can stick some hint about the order in the patch.\n\nTell me what you prefer :)\n\n-- \nMST\n"},{"id":"227789","messageId":"20130917201401.GA22000@redhat.com","threadId":"34819","inReplyTo":"xmqq38p3z5kg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T20:14:01Z","receivedAt":"2013-09-17T20:14:01Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 11:06:07AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> >> \"Michael S. Tsirkin\" writes:\n> >> \n> >> > So might it not be useful to tweak patch id to\n> >> > sort the diff, making it a bit more stable?\n> >> \n> >> That is one thing that needs to be done, I think. But it would be\n> >> unfortunate if we have to do that unconditionally, though, as we may\n> >> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> >> can do so without regressing the streaming performance for the most\n> >> common case of not using the orderfile on the generating side (hence\n> >> not having to sort on the receiving end), it would be ideal. I am\n> >> not sure offhand how much code damage we are talking about, though.\n> >\n> > So make it conditional on the presence of the orderefile option?\n> \n> That would mean that those who set orderfile from configuration in\n> the future will have to always suffer, I would think. Is that\n> acceptable? I dunno.\n> \n> Also, if the sender used a non-standard order, the recipient does\n> not know what order the patch was generated, and the recipient does\n> not use a custom orderfile, what should happen? I thought your idea\n> was to normalize by using some canonical order that is not affected\n> by the orderfile to make sure patch-id stays stable, so I would\n> imagine that such a recipient who does not have orderfile specified\n> still needs to sort before hashing, no?\n\nThinking about it some more, it's a best effort thing anyway,\ncorrect?\n\nSo how about, instead of doing a hash over the whole input,\nwe hash each chunk and XOR them together?\n\nThis way it will be stable against chunk reordering, and\nno need to keep patch in memory.\n\nHmm?\n\n-- \nMST\n"},{"id":"227786","messageId":"20130917201604.GA22008@redhat.com","threadId":"34819","inReplyTo":"20130917201401.GA22000@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T20:16:04Z","receivedAt":"2013-09-17T20:16:04Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 11:14:01PM +0300, Michael S. Tsirkin wrote:\n> On Tue, Sep 17, 2013 at 11:06:07AM -0700, Junio C Hamano wrote:\n> > \"Michael S. Tsirkin\" writes:\n> > \n> > > On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> > >> \"Michael S. Tsirkin\" writes:\n> > >> \n> > >> > So might it not be useful to tweak patch id to\n> > >> > sort the diff, making it a bit more stable?\n> > >> \n> > >> That is one thing that needs to be done, I think. But it would be\n> > >> unfortunate if we have to do that unconditionally, though, as we may\n> > >> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> > >> can do so without regressing the streaming performance for the most\n> > >> common case of not using the orderfile on the generating side (hence\n> > >> not having to sort on the receiving end), it would be ideal. I am\n> > >> not sure offhand how much code damage we are talking about, though.\n> > >\n> > > So make it conditional on the presence of the orderefile option?\n> > \n> > That would mean that those who set orderfile from configuration in\n> > the future will have to always suffer, I would think. Is that\n> > acceptable? I dunno.\n> > \n> > Also, if the sender used a non-standard order, the recipient does\n> > not know what order the patch was generated, and the recipient does\n> > not use a custom orderfile, what should happen? I thought your idea\n> > was to normalize by using some canonical order that is not affected\n> > by the orderfile to make sure patch-id stays stable, so I would\n> > imagine that such a recipient who does not have orderfile specified\n> > still needs to sort before hashing, no?\n> \n> Thinking about it some more, it's a best effort thing anyway,\n> correct?\n> \n> So how about, instead of doing a hash over the whole input,\n> we hash each chunk and XOR them together?\n> \n> This way it will be stable against chunk reordering, and\n> no need to keep patch in memory.\n> \n> Hmm?\n\nENOCOFFEE\n\nThat was a silly suggestion, two identical chunks aren't that unlikely :)\n\n> -- \n> MST\n"},{"id":"227788","messageId":"20130917201828.GC16860@sigill.intra.peff.net","threadId":"34819","inReplyTo":"20130917201604.GA22008@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-17T20:18:28Z","receivedAt":"2013-09-17T20:18:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 17, 2013 at 11:16:04PM +0300, Michael S. Tsirkin wrote:\n\n> > Thinking about it some more, it's a best effort thing anyway,\n> > correct?\n> > \n> > So how about, instead of doing a hash over the whole input,\n> > we hash each chunk and XOR them together?\n> > \n> > This way it will be stable against chunk reordering, and\n> > no need to keep patch in memory.\n> > \n> > Hmm?\n> \n> ENOCOFFEE\n> \n> That was a silly suggestion, two identical chunks aren't that unlikely :)\n\nIn a single patch, they should not be, as we should be taking into\naccount the filenames, no?\n\nYou could also do it hierarchically. Hash each chunk, store only the\nhashes, then sort them and hash the result. That still has O(chunks)\nstorage, but it is only one hash per chunk, not the whole data.\n\nA problem with both schemes, though, is that they are not\nbackwards-compatible with existing git-patch-id implementations. Whereas\nsorting the data itself is (kind of, at least with respect to people who\nare not using orderfile).\n\n-Peff\n"},{"id":"227793","messageId":"20130917203131.GA22051@redhat.com","threadId":"34819","inReplyTo":"20130917201604.GA22008@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T20:31:31Z","receivedAt":"2013-09-17T20:31:31Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 11:16:04PM +0300, Michael S. Tsirkin wrote:\n> On Tue, Sep 17, 2013 at 11:14:01PM +0300, Michael S. Tsirkin wrote:\n> > On Tue, Sep 17, 2013 at 11:06:07AM -0700, Junio C Hamano wrote:\n> > > \"Michael S. Tsirkin\" writes:\n> > > \n> > > > On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> > > >> \"Michael S. Tsirkin\" writes:\n> > > >> \n> > > >> > So might it not be useful to tweak patch id to\n> > > >> > sort the diff, making it a bit more stable?\n> > > >> \n> > > >> That is one thing that needs to be done, I think. But it would be\n> > > >> unfortunate if we have to do that unconditionally, though, as we may\n> > > >> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> > > >> can do so without regressing the streaming performance for the most\n> > > >> common case of not using the orderfile on the generating side (hence\n> > > >> not having to sort on the receiving end), it would be ideal. I am\n> > > >> not sure offhand how much code damage we are talking about, though.\n> > > >\n> > > > So make it conditional on the presence of the orderefile option?\n> > > \n> > > That would mean that those who set orderfile from configuration in\n> > > the future will have to always suffer, I would think. Is that\n> > > acceptable? I dunno.\n> > > \n> > > Also, if the sender used a non-standard order, the recipient does\n> > > not know what order the patch was generated, and the recipient does\n> > > not use a custom orderfile, what should happen? I thought your idea\n> > > was to normalize by using some canonical order that is not affected\n> > > by the orderfile to make sure patch-id stays stable, so I would\n> > > imagine that such a recipient who does not have orderfile specified\n> > > still needs to sort before hashing, no?\n> > \n> > Thinking about it some more, it's a best effort thing anyway,\n> > correct?\n> > \n> > So how about, instead of doing a hash over the whole input,\n> > we hash each chunk and XOR them together?\n> > \n> > This way it will be stable against chunk reordering, and\n> > no need to keep patch in memory.\n> > \n> > Hmm?\n> \n> ENOCOFFEE\n> \n> That was a silly suggestion, two identical chunks aren't that unlikely :)\n\nOTOH we can detect such malformed patches just by keeping the chunk\nhashes in memory...\n\n\n> > -- \n> > MST\n"},{"id":"227794","messageId":"20130917203807.GA22059@redhat.com","threadId":"34819","inReplyTo":"20130917201828.GC16860@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T20:38:07Z","receivedAt":"2013-09-17T20:38:07Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 04:18:28PM -0400, Jeff King wrote:\n> On Tue, Sep 17, 2013 at 11:16:04PM +0300, Michael S. Tsirkin wrote:\n> \n> > > Thinking about it some more, it's a best effort thing anyway,\n> > > correct?\n> > > \n> > > So how about, instead of doing a hash over the whole input,\n> > > we hash each chunk and XOR them together?\n> > > \n> > > This way it will be stable against chunk reordering, and\n> > > no need to keep patch in memory.\n> > > \n> > > Hmm?\n> > \n> > ENOCOFFEE\n> > \n> > That was a silly suggestion, two identical chunks aren't that unlikely :)\n> \n> In a single patch, they should not be, as we should be taking into\n> account the filenames, no?\n\nRight.\n\n> You could also do it hierarchically. Hash each chunk, store only the\n> hashes, then sort them and hash the result. That still has O(chunks)\n> storage, but it is only one hash per chunk, not the whole data.\n\nCould be optional too :)\nOr maybe just sum byte by byte instead.\n\n> A problem with both schemes, though, is that they are not\n> backwards-compatible with existing git-patch-id implementations.\n\nCould you clarify?\nWe never send patch IDs on the wire - how isn't this compatible?\n\n> Whereas\n> sorting the data itself is (kind of, at least with respect to people who\n> are not using orderfile).\n> \n> -Peff\n"},{"id":"227796","messageId":"20130917204159.GA22116@redhat.com","threadId":"34819","inReplyTo":"20130917203807.GA22059@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T20:41:59Z","receivedAt":"2013-09-17T20:41:59Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 11:38:07PM +0300, Michael S. Tsirkin wrote:\n> On Tue, Sep 17, 2013 at 04:18:28PM -0400, Jeff King wrote:\n> > On Tue, Sep 17, 2013 at 11:16:04PM +0300, Michael S. Tsirkin wrote:\n> > \n> > > > Thinking about it some more, it's a best effort thing anyway,\n> > > > correct?\n> > > > \n> > > > So how about, instead of doing a hash over the whole input,\n> > > > we hash each chunk and XOR them together?\n> > > > \n> > > > This way it will be stable against chunk reordering, and\n> > > > no need to keep patch in memory.\n> > > > \n> > > > Hmm?\n> > > \n> > > ENOCOFFEE\n> > > \n> > > That was a silly suggestion, two identical chunks aren't that unlikely :)\n> > \n> > In a single patch, they should not be, as we should be taking into\n> > account the filenames, no?\n> \n> Right.\n> \n> > You could also do it hierarchically. Hash each chunk, store only the\n> > hashes, then sort them and hash the result. That still has O(chunks)\n> > storage, but it is only one hash per chunk, not the whole data.\n> \n> Could be optional too :)\n> Or maybe just sum byte by byte instead.\n\nOne's complement probably ...\n\n> > A problem with both schemes, though, is that they are not\n> > backwards-compatible with existing git-patch-id implementations.\n> \n> Could you clarify?\n> We never send patch IDs on the wire - how isn't this compatible?\n> \n> > Whereas\n> > sorting the data itself is (kind of, at least with respect to people who\n> > are not using orderfile).\n> > \n> > -Peff\n"},{"id":"227798","messageId":"20130917205615.GA20178@sigill.intra.peff.net","threadId":"34819","inReplyTo":"20130917203807.GA22059@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-17T20:56:16Z","receivedAt":"2013-09-17T20:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 17, 2013 at 11:38:07PM +0300, Michael S. Tsirkin wrote:\n\n> > A problem with both schemes, though, is that they are not\n> > backwards-compatible with existing git-patch-id implementations.\n> \n> Could you clarify?\n> We never send patch IDs on the wire - how isn't this compatible?\n\nI meant that you might be comparing patch-ids generated by different\nimplementations, or across time. There are no dedicated tools to do so,\nbut it is very easy to do so with standard tools like \"join\".\n\nFor example, you can do:\n\n patch_ids() {\n git rev-list \"$1\" |\n git diff-tree --stdin -p |\n git patch-id |\n sort\n }\n\n patch_ids origin..topic1 >us\n patch_ids origin..topic2 >them\n\n join us them | cut -d' ' -f2-3\n\nto get a list of correlated commits between two branches. If the \"them\"\nwas on another machine with a different implementation (or is saved from\nan earlier time), your patch-ids would not match.\n\nIt may be esoteric enough not to worry about, though. By far the most\ncommon use of patch-ids is going to be in a single \"rev-list\n--cherry-pick\" situation where you are trying to omit commits during\na rebase.\n\nI am mostly thinking of the problems we had with the \"kup\" tool, which\nexpected stability across diffs that would be signed by both kernel.org.\nBut as far as I know, they do not use patch-id. More details in case you\nare curious (including me arguing that we should not care, and it is\nkup's problem!) are here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/192331/focus=192424\n\nrerere is mentioned in that thread, but I believe that it does its own\nhash, and does not rely on patch-id.\n\n-Peff\n"},{"id":"227800","messageId":"20130917210325.GA22511@redhat.com","threadId":"34819","inReplyTo":"20130917205615.GA20178@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-17T21:03:25Z","receivedAt":"2013-09-17T21:03:25Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 04:56:16PM -0400, Jeff King wrote:\n> On Tue, Sep 17, 2013 at 11:38:07PM +0300, Michael S. Tsirkin wrote:\n> \n> > > A problem with both schemes, though, is that they are not\n> > > backwards-compatible with existing git-patch-id implementations.\n> > \n> > Could you clarify?\n> > We never send patch IDs on the wire - how isn't this compatible?\n> \n> I meant that you might be comparing patch-ids generated by different\n> implementations, or across time. There are no dedicated tools to do so,\n> but it is very easy to do so with standard tools like \"join\".\n> \n> For example, you can do:\n> \n> patch_ids() {\n> git rev-list \"$1\" |\n> git diff-tree --stdin -p |\n> git patch-id |\n> sort\n> }\n> \n> patch_ids origin..topic1 >us\n> patch_ids origin..topic2 >them\n> \n> join us them | cut -d' ' -f2-3\n> \n> to get a list of correlated commits between two branches. If the \"them\"\n> was on another machine with a different implementation (or is saved from\n> an earlier time), your patch-ids would not match.\n> \n> It may be esoteric enough not to worry about, though. By far the most\n> common use of patch-ids is going to be in a single \"rev-list\n> --cherry-pick\" situation where you are trying to omit commits during\n> a rebase.\n> \n> I am mostly thinking of the problems we had with the \"kup\" tool, which\n> expected stability across diffs that would be signed by both kernel.org.\n> But as far as I know, they do not use patch-id.\n\nWe can always do a compatibility option. --order-sensitive ?\n--ignore-order ?\n\n> More details in case you\n> are curious (including me arguing that we should not care, and it is\n> kup's problem!) are here:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/192331/focus=192424\n> \n> rerere is mentioned in that thread, but I believe that it does its own\n> hash, and does not rely on patch-id.\n> \n> -Peff\n\nI think so too.\n"},{"id":"227801","messageId":"20130917210626.GE16860@sigill.intra.peff.net","threadId":"34819","inReplyTo":"20130917210325.GA22511@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-17T21:06:26Z","receivedAt":"2013-09-17T21:06:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 18, 2013 at 12:03:25AM +0300, Michael S. Tsirkin wrote:\n\n> > It may be esoteric enough not to worry about, though. By far the most\n> > common use of patch-ids is going to be in a single \"rev-list\n> > --cherry-pick\" situation where you are trying to omit commits during\n> > a rebase.\n> > \n> > I am mostly thinking of the problems we had with the \"kup\" tool, which\n> > expected stability across diffs that would be signed by both kernel.org.\n> > But as far as I know, they do not use patch-id.\n> \n> We can always do a compatibility option. --order-sensitive ?\n> --ignore-order ?\n\nThat may make sense as an escape hatch in case somebody has a use we\ndidn't foresee.\n\nIf it is just about \"consistent order\" versus \"whatever is in the diff\",\nI do not know that we need to worry as much; only the minority using\norderfile is affected, and they have _always_ been affected. IOW, we are\nfixing a bug, and they should be happier.\n\nBut if it is changing the output entirely in all cases (e.g., the\n1s-complement sum), I think you would want to have a \"classic\" mode that\ntries to be compatible with the old style (with the caveat that of\ncourse it depends on patch ordering).\n\n-Peff\n"},{"id":"227805","messageId":"xmqqob7rw1xt.fsf@gitster.dls.corp.google.com","threadId":"34819","inReplyTo":"20130917205615.GA20178@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T21:52:45Z","receivedAt":"2013-09-17T21:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I am mostly thinking of the problems we had with the \"kup\" tool, which\n> expected stability across diffs that would be signed by both kernel.org.\n> But as far as I know, they do not use patch-id. More details in case you\n> are curious (including me arguing that we should not care, and it is\n> kup's problem!) are here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/192331/focus=192424\n>\n> rerere is mentioned in that thread, but I believe that it does its own\n> hash, and does not rely on patch-id.\n\nIt does not. The reason rerere is relevant in that thread but is\nnot directly relevant here is because the thread is about changing\nthe diff algorithm, hence changing the common-sequence matches\nbetween preimage and postimage, resulting in a different shape of\nthe conflicted section. The hash rerere does still depends on the\npatch text, but it is restricted to the conflicted region, so\nwhatever we do in patch-id will not affect it. Also the rerere\ndatabase is not keyed by pathname (a conflict in _any_ file as long\nas they have the same ours/theirs conflicted text is recognised as\nthe same conflict we have already seen), so changing the orderfile\nwould not affect it at all.\n"},{"id":"227907","messageId":"20130919213226.GA21291@redhat.com","threadId":"34819","inReplyTo":"20130917205615.GA20178@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-19T21:32:26Z","receivedAt":"2013-09-19T21:32:26Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 04:56:16PM -0400, Jeff King wrote:\n> On Tue, Sep 17, 2013 at 11:38:07PM +0300, Michael S. Tsirkin wrote:\n> \n> > > A problem with both schemes, though, is that they are not\n> > > backwards-compatible with existing git-patch-id implementations.\n> > \n> > Could you clarify?\n> > We never send patch IDs on the wire - how isn't this compatible?\n> \n> I meant that you might be comparing patch-ids generated by different\n> implementations, or across time. There are no dedicated tools to do so,\n> but it is very easy to do so with standard tools like \"join\".\n> \n> For example, you can do:\n> \n> patch_ids() {\n> git rev-list \"$1\" |\n> git diff-tree --stdin -p |\n> git patch-id |\n> sort\n> }\n> \n> patch_ids origin..topic1 >us\n> patch_ids origin..topic2 >them\n> \n> join us them | cut -d' ' -f2-3\n> \n> to get a list of correlated commits between two branches. If the \"them\"\n> was on another machine with a different implementation (or is saved from\n> an earlier time), your patch-ids would not match.\n> \n> It may be esoteric enough not to worry about, though. By far the most\n> common use of patch-ids is going to be in a single \"rev-list\n> --cherry-pick\" situation where you are trying to omit commits during\n> a rebase.\n> \n> I am mostly thinking of the problems we had with the \"kup\" tool, which\n> expected stability across diffs that would be signed by both kernel.org.\n> But as far as I know, they do not use patch-id. More details in case you\n> are curious (including me arguing that we should not care, and it is\n> kup's problem!) are here:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/192331/focus=192424\n> \n> rerere is mentioned in that thread, but I believe that it does its own\n> hash, and does not rely on patch-id.\n> \n> -Peff\n\nOK so far I came up with the following.\nNeeds documentation and tests obviously.\nBut besides that -\nwould something like this be enough to address the\nissue Junio raised?\n\n--->\n\npatch-id: make it more stable\n\nAdd a new patch-id algorithm making it stable against\nhunk reodering:\n\t- prepend header to each hunk (if not there)\n\t- calculate SHA1 hash for each hunk separately\n\t- sum all hashes to get patch id\n\nAdd --order-sensitive to get historical unstable behaviour.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/patch-id.c | 65 +++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 50 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3cfe02d..d1902ff 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -1,17 +1,14 @@\n #include \"builtin.h\"\n \n-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)\n+static void flush_current_id(int patchlen, unsigned char *id, unsigned char *result)\n {\n-\tunsigned char result[20];\n \tchar name[50];\n \n \tif (!patchlen)\n \t\treturn;\n \n-\tgit_SHA1_Final(result, c);\n \tmemcpy(name, sha1_to_hex(id), 41);\n \tprintf(\"%s %s\\n\", sha1_to_hex(result), name);\n-\tgit_SHA1_Init(c);\n }\n \n static int remove_space(char *line)\n@@ -56,10 +53,30 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct strbuf *line_buf)\n+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)\n {\n-\tint patchlen = 0, found_next = 0;\n+\tunsigned char hash[20];\n+\tunsigned short carry = 0;\n+\tint i;\n+\n+\tgit_SHA1_Final(hash, ctx);\n+\tgit_SHA1_Init(ctx);\n+\t/* 20-byte sum, with carry */\n+\tfor (i = 0; i < 20; ++i) {\n+\t\tcarry += result[i] + hash[i];\n+\t\tresult[i] = carry;\n+\t\tcarry >>= 8;\n+\t}\n+}\n+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n+\t\t\t struct strbuf *line_buf, int stable)\n+{\n+\tint patchlen = 0, found_next = 0, hunks = 0;\n \tint before = -1, after = -1;\n+\tgit_SHA_CTX ctx, header_ctx;\n+\n+\tgit_SHA1_Init(&ctx);\n+\thashclr(result);\n \n \twhile (strbuf_getwholeline(line_buf, stdin, '\\n') != EOF) {\n \t\tchar *line = line_buf->buf;\n@@ -99,6 +116,18 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\t\tif (!memcmp(line, \"@@ -\", 4)) {\n \t\t\t\t/* Parse next hunk, but ignore line numbers. */\n \t\t\t\tscan_hunk_header(line, &before, &after);\n+\t\t\t\tif (stable) {\n+\t\t\t\t\tif (hunks) {\n+\t\t\t\t\t\tflush_one_hunk(result, &ctx);\n+\t\t\t\t\t\tmemcpy(&ctx, &header_ctx,\n+\t\t\t\t\t\t sizeof ctx);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\t/* Save ctx for next hunk. */\n+\t\t\t\t\t\tmemcpy(&header_ctx, &ctx,\n+\t\t\t\t\t\t sizeof ctx);\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t\thunks++;\n \t\t\t\tcontinue;\n \t\t\t}\n \n@@ -108,6 +137,7 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \n \t\t\t/* Else we're parsing another header. */\n \t\t\tbefore = after = -1;\n+\t\t\thunks = 0;\n \t\t}\n \n \t\t/* If we get here, we're inside a hunk. */\n@@ -119,27 +149,27 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\t/* Compute the sha without whitespace */\n \t\tlen = remove_space(line);\n \t\tpatchlen += len;\n-\t\tgit_SHA1_Update(ctx, line, len);\n+\t\tgit_SHA1_Update(&ctx, line, len);\n \t}\n \n \tif (!found_next)\n \t\thashclr(next_sha1);\n \n+\tflush_one_hunk(result, &ctx);\n+\n \treturn patchlen;\n }\n \n-static void generate_id_list(void)\n+static void generate_id_list(int stable)\n {\n-\tunsigned char sha1[20], n[20];\n-\tgit_SHA_CTX ctx;\n+\tunsigned char sha1[20], n[20], result[20];\n \tint patchlen;\n \tstruct strbuf line_buf = STRBUF_INIT;\n \n-\tgit_SHA1_Init(&ctx);\n \thashclr(sha1);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(n, &ctx, &line_buf);\n-\t\tflush_current_id(patchlen, sha1, &ctx);\n+\t\tpatchlen = get_one_patchid(n, result, &line_buf, stable);\n+\t\tflush_current_id(patchlen, sha1, result);\n \t\thashcpy(sha1, n);\n \t}\n \tstrbuf_release(&line_buf);\n@@ -149,9 +179,14 @@ static const char patch_id_usage[] = \"git patch-id < patch\";\n \n int cmd_patch_id(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc != 1)\n+\tint stable;\n+\tif (argc == 2 && !strcmp(argv[1], \"--order-sensitive\"))\n+\t\tstable = 0;\n+\telse if (argc == 1)\n+\t\tstable = 1;\n+\telse\n \t\tusage(patch_id_usage);\n \n-\tgenerate_id_list();\n+\tgenerate_id_list(stable);\n \treturn 0;\n }\n-- \nMST\n"},{"id":"228014","messageId":"20130921210839.GA21974@redhat.com","threadId":"34819","inReplyTo":"xmqqfvt3z7i4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-21T21:08:39Z","receivedAt":"2013-09-21T21:08:39Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 17, 2013 at 10:24:19AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > So might it not be useful to tweak patch id to\n> > sort the diff, making it a bit more stable?\n> \n> That is one thing that needs to be done, I think. But it would be\n> unfortunate if we have to do that unconditionally, though, as we may\n> be \"buffering\" many hundred kilobytes of patch text in core. If we\n> can do so without regressing the streaming performance for the most\n> common case of not using the orderfile on the generating side (hence\n> not having to sort on the receiving end), it would be ideal. I am\n> not sure offhand how much code damage we are talking about, though.\n> \n> > I'll be glad to help do this if you tell me what these parts are.\n> > anything else besides fixing besides the stand-alone patch id?\n> \n> Off the top of my head I do not think of one (but that is not a\n> guarantee that there isn't any).\n\nOkay so I did this by reworking the internal algorithm used by the\nstand-alone patch-id (hope you've seen this mail).\nNow, a question: does it matter whether the algorithm used by diff_get_patch_id\nis different?\nDoes something rely on them being the same?\n\nIf yes, we'd have to change diff_get_patch_id as well.\n"},{"id":"228103","messageId":"20130923210915.GA11202@redhat.com","threadId":"34819","inReplyTo":"20130919213226.GA21291@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-23T21:09:15Z","receivedAt":"2013-09-23T21:09:15Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Fri, Sep 20, 2013 at 12:32:26AM +0300, Michael S. Tsirkin wrote:\n> On Tue, Sep 17, 2013 at 04:56:16PM -0400, Jeff King wrote:\n> > On Tue, Sep 17, 2013 at 11:38:07PM +0300, Michael S. Tsirkin wrote:\n> > \n> > > > A problem with both schemes, though, is that they are not\n> > > > backwards-compatible with existing git-patch-id implementations.\n> > > \n> > > Could you clarify?\n> > > We never send patch IDs on the wire - how isn't this compatible?\n> > \n> > I meant that you might be comparing patch-ids generated by different\n> > implementations, or across time. There are no dedicated tools to do so,\n> > but it is very easy to do so with standard tools like \"join\".\n> > \n> > For example, you can do:\n> > \n> > patch_ids() {\n> > git rev-list \"$1\" |\n> > git diff-tree --stdin -p |\n> > git patch-id |\n> > sort\n> > }\n> > \n> > patch_ids origin..topic1 >us\n> > patch_ids origin..topic2 >them\n> > \n> > join us them | cut -d' ' -f2-3\n> > \n> > to get a list of correlated commits between two branches. If the \"them\"\n> > was on another machine with a different implementation (or is saved from\n> > an earlier time), your patch-ids would not match.\n> > \n> > It may be esoteric enough not to worry about, though. By far the most\n> > common use of patch-ids is going to be in a single \"rev-list\n> > --cherry-pick\" situation where you are trying to omit commits during\n> > a rebase.\n> > \n> > I am mostly thinking of the problems we had with the \"kup\" tool, which\n> > expected stability across diffs that would be signed by both kernel.org.\n> > But as far as I know, they do not use patch-id. More details in case you\n> > are curious (including me arguing that we should not care, and it is\n> > kup's problem!) are here:\n> > \n> > http://thread.gmane.org/gmane.comp.version-control.git/192331/focus=192424\n> > \n> > rerere is mentioned in that thread, but I believe that it does its own\n> > hash, and does not rely on patch-id.\n> > \n> > -Peff\n> \n> OK so far I came up with the following.\n> Needs documentation and tests obviously.\n> But besides that -\n> would something like this be enough to address the\n> issue Junio raised?\n\nPing. Junio could you comment on this please?\n\n> --->\n> \n> patch-id: make it more stable\n> \n> Add a new patch-id algorithm making it stable against\n> hunk reodering:\n> \t- prepend header to each hunk (if not there)\n> \t- calculate SHA1 hash for each hunk separately\n> \t- sum all hashes to get patch id\n> \n> Add --order-sensitive to get historical unstable behaviour.\n> \n> Signed-off-by: Michael S. Tsirkin \n> ---\n> builtin/patch-id.c | 65 +++++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 50 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 3cfe02d..d1902ff 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\n> @@ -1,17 +1,14 @@\n> #include \"builtin.h\"\n> \n> -static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)\n> +static void flush_current_id(int patchlen, unsigned char *id, unsigned char *result)\n> {\n> -\tunsigned char result[20];\n> \tchar name[50];\n> \n> \tif (!patchlen)\n> \t\treturn;\n> \n> -\tgit_SHA1_Final(result, c);\n> \tmemcpy(name, sha1_to_hex(id), 41);\n> \tprintf(\"%s %s\\n\", sha1_to_hex(result), name);\n> -\tgit_SHA1_Init(c);\n> }\n> \n> static int remove_space(char *line)\n> @@ -56,10 +53,30 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n> \treturn 1;\n> }\n> \n> -static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct strbuf *line_buf)\n> +static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)\n> {\n> -\tint patchlen = 0, found_next = 0;\n> +\tunsigned char hash[20];\n> +\tunsigned short carry = 0;\n> +\tint i;\n> +\n> +\tgit_SHA1_Final(hash, ctx);\n> +\tgit_SHA1_Init(ctx);\n> +\t/* 20-byte sum, with carry */\n> +\tfor (i = 0; i < 20; ++i) {\n> +\t\tcarry += result[i] + hash[i];\n> +\t\tresult[i] = carry;\n> +\t\tcarry >>= 8;\n> +\t}\n> +}\n> +static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n> +\t\t\t struct strbuf *line_buf, int stable)\n> +{\n> +\tint patchlen = 0, found_next = 0, hunks = 0;\n> \tint before = -1, after = -1;\n> +\tgit_SHA_CTX ctx, header_ctx;\n> +\n> +\tgit_SHA1_Init(&ctx);\n> +\thashclr(result);\n> \n> \twhile (strbuf_getwholeline(line_buf, stdin, '\\n') != EOF) {\n> \t\tchar *line = line_buf->buf;\n> @@ -99,6 +116,18 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n> \t\t\tif (!memcmp(line, \"@@ -\", 4)) {\n> \t\t\t\t/* Parse next hunk, but ignore line numbers. */\n> \t\t\t\tscan_hunk_header(line, &before, &after);\n> +\t\t\t\tif (stable) {\n> +\t\t\t\t\tif (hunks) {\n> +\t\t\t\t\t\tflush_one_hunk(result, &ctx);\n> +\t\t\t\t\t\tmemcpy(&ctx, &header_ctx,\n> +\t\t\t\t\t\t sizeof ctx);\n> +\t\t\t\t\t} else {\n> +\t\t\t\t\t\t/* Save ctx for next hunk. */\n> +\t\t\t\t\t\tmemcpy(&header_ctx, &ctx,\n> +\t\t\t\t\t\t sizeof ctx);\n> +\t\t\t\t\t}\n> +\t\t\t\t}\n> +\t\t\t\thunks++;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \n> @@ -108,6 +137,7 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n> \n> \t\t\t/* Else we're parsing another header. */\n> \t\t\tbefore = after = -1;\n> +\t\t\thunks = 0;\n> \t\t}\n> \n> \t\t/* If we get here, we're inside a hunk. */\n> @@ -119,27 +149,27 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n> \t\t/* Compute the sha without whitespace */\n> \t\tlen = remove_space(line);\n> \t\tpatchlen += len;\n> -\t\tgit_SHA1_Update(ctx, line, len);\n> +\t\tgit_SHA1_Update(&ctx, line, len);\n> \t}\n> \n> \tif (!found_next)\n> \t\thashclr(next_sha1);\n> \n> +\tflush_one_hunk(result, &ctx);\n> +\n> \treturn patchlen;\n> }\n> \n> -static void generate_id_list(void)\n> +static void generate_id_list(int stable)\n> {\n> -\tunsigned char sha1[20], n[20];\n> -\tgit_SHA_CTX ctx;\n> +\tunsigned char sha1[20], n[20], result[20];\n> \tint patchlen;\n> \tstruct strbuf line_buf = STRBUF_INIT;\n> \n> -\tgit_SHA1_Init(&ctx);\n> \thashclr(sha1);\n> \twhile (!feof(stdin)) {\n> -\t\tpatchlen = get_one_patchid(n, &ctx, &line_buf);\n> -\t\tflush_current_id(patchlen, sha1, &ctx);\n> +\t\tpatchlen = get_one_patchid(n, result, &line_buf, stable);\n> +\t\tflush_current_id(patchlen, sha1, result);\n> \t\thashcpy(sha1, n);\n> \t}\n> \tstrbuf_release(&line_buf);\n> @@ -149,9 +179,14 @@ static const char patch_id_usage[] = \"git patch-id < patch\";\n> \n> int cmd_patch_id(int argc, const char **argv, const char *prefix)\n> {\n> -\tif (argc != 1)\n> +\tint stable;\n> +\tif (argc == 2 && !strcmp(argv[1], \"--order-sensitive\"))\n> +\t\tstable = 0;\n> +\telse if (argc == 1)\n> +\t\tstable = 1;\n> +\telse\n> \t\tusage(patch_id_usage);\n> \n> -\tgenerate_id_list();\n> +\tgenerate_id_list(stable);\n> \treturn 0;\n> }\n> -- \n> MST\n> \n"},{"id":"228111","messageId":"20130923213729.GE9464@google.com","threadId":"34819","inReplyTo":"20130923210915.GA11202@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-23T21:37:29Z","receivedAt":"2013-09-23T21:37:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael S. Tsirkin wrote:\n>> On Tue, Sep 17, 2013 at 04:56:16PM -0400, Jeff King wrote:\n\n>>>>> A problem with both schemes, though, is that they are not\n>>>>> backwards-compatible with existing git-patch-id implementations.\n[...]\n>>> It may be esoteric enough not to worry about, though.\n\nYeah, I think it would be okay. Details of the diff generation\nalgorithm have changed from time to time anyway (and broken things,\nas you mentioned) and we make no guarantee about this.\n\n[...]\n>> patch-id: make it more stable\n>>\n>> Add a new patch-id algorithm making it stable against\n>> hunk reodering:\n>> \t- prepend header to each hunk (if not there)\n>> \t- calculate SHA1 hash for each hunk separately\n>> \t- sum all hashes to get patch id\n>>\n>> Add --order-sensitive to get historical unstable behaviour.\n\nThe --order-sensitive option seems confusing. How do I use it to\nreplicate a historical patch-id? If I record all options that might\nhave influenced ordering (which are those?) then am I guaranteed to\nget a reproducible result? \n\nSo I would prefer either of the following over the above:\n\n a) When asked to compute the patch-id of a seekable file, use the\n current streaming implementation until you notice a filename that\n is out of order. Then start over with sorted hunks (for example\n building a table of offsets within the patch for each hunk to\n support this).\n\n When asked to compute the patch-id of an unseekable file, stream\n to a temporary file under $GIT_DIR to get a seekable file.\n\n b) Unconditionally use the new patch-id definition that is stable\n under permutation of hunks. If and when someone complains that\n this invalidates their old patch-ids, they can work on adding a\n nice interface for getting the old-style patch-ids. I suspect it\n just wouldn't come up.\n\nOf course I can easily be wrong. Thanks for a clear patch that makes\nthe choices easy to reasonable about.\n\nThoughts?\nJonathan\n"},{"id":"228129","messageId":"20130924054520.GC5875@sigill.intra.peff.net","threadId":"34819","inReplyTo":"20130923213729.GE9464@google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-24T05:45:21Z","receivedAt":"2013-09-24T05:45:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n\n> >> Add --order-sensitive to get historical unstable behaviour.\n> \n> The --order-sensitive option seems confusing. How do I use it to\n> replicate a historical patch-id? If I record all options that might\n> have influenced ordering (which are those?) then am I guaranteed to\n> get a reproducible result? \n\nYes, I have the same complaint. I'd much rather the classic mode be\ngiven a name of some sort, and then have a \"--patch-id-mode=classic\"\nparameter (or probably some better name) that sets all of the\nparameters. Then you know that two implementations using \"classic\"\nshould get the same output, and so forth if we have to tweak it again.\n\n> So I would prefer either of the following over the above:\n> \n> a) When asked to compute the patch-id of a seekable file, use the\n> current streaming implementation until you notice a filename that\n> is out of order. Then start over with sorted hunks (for example\n> building a table of offsets within the patch for each hunk to\n> support this).\n> \n> When asked to compute the patch-id of an unseekable file, stream\n> to a temporary file under $GIT_DIR to get a seekable file.\n\nThis would mean that everybody, whether they care about compatibility or\nnot, would have to pay the price to spool to $GIT_DIR, right? That's not\ngreat, as most cases would not care.\n\n> b) Unconditionally use the new patch-id definition that is stable\n> under permutation of hunks. If and when someone complains that\n> this invalidates their old patch-ids, they can work on adding a\n> nice interface for getting the old-style patch-ids. I suspect it\n> just wouldn't come up.\n\nI think I'd prefer this one. The \"--patch-id-mode\" above is how I would\ndo it, and in general when we are potentially breaking compatibility,\nit's nice to anticipate and give an escape hatch. But I do find it\nreasonably unlikely that this will come up, so maybe this is a good time\nto practice YAGNI.\n\n-Peff\n"},{"id":"228131","messageId":"20130924055419.GA11208@redhat.com","threadId":"34819","inReplyTo":"20130923213729.GE9464@google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-24T05:54:19Z","receivedAt":"2013-09-24T05:54:19Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Michael S. Tsirkin wrote:\n> >> On Tue, Sep 17, 2013 at 04:56:16PM -0400, Jeff King wrote:\n> \n> >>>>> A problem with both schemes, though, is that they are not\n> >>>>> backwards-compatible with existing git-patch-id implementations.\n> [...]\n> >>> It may be esoteric enough not to worry about, though.\n> \n> Yeah, I think it would be okay. Details of the diff generation\n> algorithm have changed from time to time anyway (and broken things,\n> as you mentioned) and we make no guarantee about this.\n> \n> [...]\n> >> patch-id: make it more stable\n> >>\n> >> Add a new patch-id algorithm making it stable against\n> >> hunk reodering:\n> >> \t- prepend header to each hunk (if not there)\n> >> \t- calculate SHA1 hash for each hunk separately\n> >> \t- sum all hashes to get patch id\n> >>\n> >> Add --order-sensitive to get historical unstable behaviour.\n> \n> The --order-sensitive option seems confusing. How do I use it to\n> replicate a historical patch-id?\n\nYou supply a historical diff to it :)\n\n> If I record all options that might\n> have influenced ordering (which are those?) then am I guaranteed to\n> get a reproducible result? \n\nMaybe not. But if you have a patch on disk, you will get\nold hash from it with --order-sensitive.\n\n> So I would prefer either of the following over the above:\n> \n> a) When asked to compute the patch-id of a seekable file, use the\n> current streaming implementation until you notice a filename that\n> is out of order. Then start over with sorted hunks (for example\n> building a table of offsets within the patch for each hunk to\n> support this).\n> \n> When asked to compute the patch-id of an unseekable file, stream\n> to a temporary file under $GIT_DIR to get a seekable file.\n\nThis can be computed in one pass: just keep two checksums around.\n\nBut the result won't be stable: if you get same patch from two\npeople one is ordered, the other isn't, you get two different checksums.\n\nWhat are we trying to achieve here?\n\n\n> b) Unconditionally use the new patch-id definition that is stable\n> under permutation of hunks. If and when someone complains that\n> this invalidates their old patch-ids, they can work on adding a\n> nice interface for getting the old-style patch-ids. I suspect it\n> just wouldn't come up.\n\nThat's certainly easy to implement.\n\n> Of course I can easily be wrong. Thanks for a clear patch that makes\n> the choices easy to reasonable about.\n> \n> Thoughts?\n> Jonathan\n"},{"id":"228172","messageId":"20130924193610.GO9464@google.com","threadId":"34819","inReplyTo":"20130924055419.GA11208@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-24T19:36:10Z","receivedAt":"2013-09-24T19:36:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n> On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n\n>> a) When asked to compute the patch-id of a seekable file, use the\n>> current streaming implementation until you notice a filename that\n>> is out of order. Then start over with sorted hunks (for example\n>> building a table of offsets within the patch for each hunk to\n>> support this).\n>>\n>> When asked to compute the patch-id of an unseekable file, stream\n>> to a temporary file under $GIT_DIR to get a seekable file.\n>\n> This can be computed in one pass: just keep two checksums around.\n>\n> But the result won't be stable: if you get same patch from two\n> people one is ordered, the other isn't, you get two different checksums.\n\nSorry for the lack of clarity. In this case (a), I meant \"sort the\ndiff hunks and use the *old* patch-id definition with sorted hunks,\nwhich produces a stable result but requires random access.\n\nHowever:\n\n[...]\n>> b) Unconditionally use the new patch-id definition that is stable\n>> under permutation of hunks. If and when someone complains that\n>> this invalidates their old patch-ids, they can work on adding a\n>> nice interface for getting the old-style patch-ids. I suspect it\n>> just wouldn't come up.\n>\n> That's certainly easy to implement.\n\nYes, (b) seems like the best option to me, for what it's worth.\n\nThanks again,\nJonathan\n"},{"id":"228174","messageId":"20130924201515.GB23319@redhat.com","threadId":"34819","inReplyTo":"20130924193610.GO9464@google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-24T20:15:15Z","receivedAt":"2013-09-24T20:15:15Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 24, 2013 at 12:36:10PM -0700, Jonathan Nieder wrote:\n> Michael S. Tsirkin wrote:\n> > On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n> \n> >> a) When asked to compute the patch-id of a seekable file, use the\n> >> current streaming implementation until you notice a filename that\n> >> is out of order. Then start over with sorted hunks (for example\n> >> building a table of offsets within the patch for each hunk to\n> >> support this).\n> >>\n> >> When asked to compute the patch-id of an unseekable file, stream\n> >> to a temporary file under $GIT_DIR to get a seekable file.\n> >\n> > This can be computed in one pass: just keep two checksums around.\n> >\n> > But the result won't be stable: if you get same patch from two\n> > people one is ordered, the other isn't, you get two different checksums.\n> \n> Sorry for the lack of clarity. In this case (a), I meant \"sort the\n> diff hunks and use the *old* patch-id definition with sorted hunks,\n\nWell, then the result is not compatible with what\noriginal patch-id would produce.\nBasically you are either not stable or not compatible :)\n\n> which produces a stable result but requires random access.\n> \n> However:\n> \n> [...]\n> >> b) Unconditionally use the new patch-id definition that is stable\n> >> under permutation of hunks. If and when someone complains that\n> >> this invalidates their old patch-ids, they can work on adding a\n> >> nice interface for getting the old-style patch-ids. I suspect it\n> >> just wouldn't come up.\n> >\n> > That's certainly easy to implement.\n> \n> Yes, (b) seems like the best option to me, for what it's worth.\n> \n> Thanks again,\n> Jonathan\n\nOK, thanks.\n\nJust making sure: is it correct that there's no requirement to use same\nalgorithm between patch-ids.c and builtin/patch-id.c ?\n"},{"id":"228177","messageId":"20130924213116.GQ9464@google.com","threadId":"34819","inReplyTo":"20130924201515.GB23319@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-24T21:31:16Z","receivedAt":"2013-09-24T21:31:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n>>> On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n\n>>>> Then start over with sorted hunks (for example\n>>>> building a table of offsets within the patch for each hunk to\n>>>> support this).\n[...]\n> Well, then the result is not compatible with what\n> original patch-id would produce.\n\nNope, I meant sorting to produce what the original patch-id would\nproduce for a diff with the default sorting order. The result is a\npatch-id that can be compared with patch-ids from earlier versions of\ngit as long as -O was not used (which was already not\ncompatible with reliable use of patch-id).\n\n[...]\n> Just making sure: is it correct that there's no requirement to use same\n> algorithm between patch-ids.c and builtin/patch-id.c ?\n\nI think so, as long as Documentation/git-cherry.txt is updated to stop\npretending 'git cherry' calls 'git patch-id' and the two get comments\nabout it, though it seems simpler to keep them roughly the same.\n(They already differ in handling of binary files.)\n\nThanks,\nJonathan\n"},{"id":"228178","messageId":"20130924215721.GA2072@redhat.com","threadId":"34819","inReplyTo":"20130924213116.GQ9464@google.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2013-09-24T21:57:21Z","receivedAt":"2013-09-24T21:57:21Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Tue, Sep 24, 2013 at 02:31:16PM -0700, Jonathan Nieder wrote:\n> Michael S. Tsirkin wrote:\n> >>> On Mon, Sep 23, 2013 at 02:37:29PM -0700, Jonathan Nieder wrote:\n> \n> >>>> Then start over with sorted hunks (for example\n> >>>> building a table of offsets within the patch for each hunk to\n> >>>> support this).\n> [...]\n> > Well, then the result is not compatible with what\n> > original patch-id would produce.\n> \n> Nope, I meant sorting to produce what the original patch-id would\n> produce for a diff with the default sorting order. The result is a\n> patch-id that can be compared with patch-ids from earlier versions of\n> git as long as -O was not used (which was already not\n> compatible with reliable use of patch-id).\n> \n> [...]\n> > Just making sure: is it correct that there's no requirement to use same\n> > algorithm between patch-ids.c and builtin/patch-id.c ?\n> \n> I think so, as long as Documentation/git-cherry.txt is updated to stop\n> pretending 'git cherry' calls 'git patch-id' and the two get comments\n> about it, though it seems simpler to keep them roughly the same.\n> (They already differ in handling of binary files.)\n\nHow do they differ btw?\n\n> Thanks,\n> Jonathan\n"},{"id":"228180","messageId":"20130924220849.GS9464@google.com","threadId":"34819","inReplyTo":"20130924215721.GA2072@redhat.com","subject":"Re: [PATCH] diff: add a config option to control orderfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-24T22:08:49Z","receivedAt":"2013-09-24T22:08:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n> On Tue, Sep 24, 2013 at 02:31:16PM -0700, Jonathan Nieder wrote:\n>> Michael S. Tsirkin wrote:\n\n>>> Just making sure: is it correct that there's no requirement to use same\n>>> algorithm between patch-ids.c and builtin/patch-id.c ?\n>>\n>> I think so,\n[...]\n>> (They already differ in handling of binary files.)\n>\n> How do they differ btw?\n\n\"git diff\" without --binary writes \"Binary files a/foo and b/foo\ndiffer\" and there is nothing for 'git patch-id' to consume for that\nfile. With --binary, \"git diff\" produces entries in 'GIT binary\npatch' format which patch-id also skips.\n\n'git cherry' and friends hash in the old and new versions of a binary\nfile to avoid accidentally dropping changes (see v1.5.3-rc6~9 \"Take\nbinary diffs into accounts for 'git rebase', 2007-08-18, and\nv1.7.3-rc0~27^2 \"hash binary sha1 into patch id\", 2010-08-15).\n\nJonathan\n"}]}