{"thread":{"id":"44029","subject":"[RFC/PATCH 0/2] more patch-id speedups","startedAt":"2016-09-07T07:53:58Z","lastAt":"2016-09-25T18:25:51Z","messageCount":35,"participants":["Jeff King","Johannes Schindelin","Jacob Keller","Josh Triplett","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"301242","messageId":"20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net","threadId":"44029","inReplyTo":null,"subject":"[RFC/PATCH 0/2] more patch-id speedups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T07:53:46Z","receivedAt":"2016-09-07T07:53:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Michael and I found a case where the \"format-patch --cherry-pick A...B\"\ncommand for a rebase took over 7 minutes to run with git v2.9.3. Yikes.\n\nSwitching to v2.10 dropped that to a bit over 3 minutes (due to the\nkw/patch-ids-optim topic). Better, but not great.\n\nThe culprit turned out to be merge commits; the patch-id code will\nhappily diff a merge against its first parent, and ignore the rest. This\n_seems_ like a bad idea, but maybe there is something clever going on\nthat I don't know about. I couldn't find anything useful in the history,\nand given that this code was adapted from rebase, my guess is that it\nwas never really intended to handle merge commits in the first place (of\ncourse we weren't trying to rebase merge commits; but it has to generate\npatch-ids for everything that happened on \"A\" to compare against).\n\nDropping the computation of the merge commits got it down to about 4\nseconds. I also noticed that it was doing rename detection (which also\nseems like a bad idea). Disabling renames dropped another half second or\nso.\n\nThis is marked as \"RFC\" because I don't feel entirely confident that I'm\nnot missing some clever need for these options. But in both cases my gut\nfeeling is that they are simply unintended effects that nobody ever\nnoticed, because it would be very rare that they would affect the\noutput. And that if they _did_ affect the output, they would probably be\ndoing the wrong thing.\n\n-peff\n"},{"id":"301243","messageId":"20160907075411.cr67cbvmy7gxv2om@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net","subject":"[PATCH 1/2] patch-ids: turn off rename detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T07:54:11Z","receivedAt":"2016-09-07T07:54:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-id code may be running inside another porcelain\nlike \"git log\" or \"git format-patch\", and therefore may have\nset diff_detect_rename_default, either via the diff-ui\nconfig, or by default since 5404c11 (diff: activate\ndiff.renames by default, 2016-02-25). This is the case even\nif a command is run with `--no-renames`, as that is applied\nonly to the diff-options used by the command itself.\n\nRename detection doesn't help the patch-id results. It\n_may_ actually hurt, as minor differences in the files that\nwould be overlooked by patch-id's canonicalization might\nresult in different renames (though I'd doubt that it ever\ncomes up in practice).\n\nBut mostly it is just a waste of CPU to compute these\nrenames.\n\nNote that we don't have to worry about compatibility here.\nThis patch disables renames just for the internal patch-id\ncomparison run by \"log --cherry-pick\", etc. The user-visible\n\"git patch-id\" output depends on the patch that it is fed\n(so it is up to the diff generator to use --no-renames if\nthey wish).\n\nSigned-off-by: Jeff King \n---\n patch-ids.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 082412a..77e4663 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -45,6 +45,7 @@ int init_patch_ids(struct patch_ids *ids)\n {\n \tmemset(ids, 0, sizeof(*ids));\n \tdiff_setup(&ids->diffopts);\n+\tids->diffopts.detect_rename = 0;\n \tDIFF_OPT_SET(&ids->diffopts, RECURSIVE);\n \tdiff_setup_done(&ids->diffopts);\n \thashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301244","messageId":"20160907075425.6jgamuwm66xbklu3@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net","subject":"[PATCH 2/2] patch-ids: skip merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T07:54:25Z","receivedAt":"2016-09-07T07:54:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-ids code which powers \"log --cherry-pick\" doesn't\nlook at whether each commit is a merge or not. It just feeds\nthe commit's first parent to the diff, and ignores any\nadditional parents.\n\nIn theory, this might be useful if you wanted to find\nequivalence between, say, a merge commit and a squash-merge\nthat does the same thing. But that may also be the wrong\nthing; the diffs may be the same, but the meaning of the two\ncommits is definitely not identical. We should err on the\nside of _not_ matching such commits.\n\nMoreover, we may spend a lot of extra time computing these\nmerge diffs. In the case that inspired this patch, a \"git\nformat-patch --cherry-pick\" dropped from over 3 minutes to\nless than 4 seconds. This seems pretty drastic, but is\neasily explained. The command was invoked by a \"git rebase\"\nof an older topic branch; there had been tens of thousands\nof commits on the upstream branch in the meantime. In\naddition, this project used a topic-branch workflow with\noccasional \"back-merges\" from \"master\" to each topic (to\nresolve conflicts on the topics rather than in the merge\ncommits). So there were not only extra merges, but the diffs\nfor these back-merges were generally quite large (because\nthey represented _everything_ that had been merged to master\nsince the topic branched).\n\nThis patch just ignores merge commits entirely when\ngenerating patch-ids, meaning they will never be matched\n(from either side of a symmetric-diff traversal).\n\nSigned-off-by: Jeff King \n---\n patch-ids.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 77e4663..b1f8514 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -7,10 +7,12 @@\n int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t unsigned char *sha1, int diff_header_only)\n {\n-\tif (commit->parents)\n+\tif (commit->parents) {\n+\t\tif (commit->parents->next)\n+\t\t\treturn 0;\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n \t\t\t commit->object.oid.hash, \"\", options);\n-\telse\n+\t} else\n \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n \tdiffcore_std(options);\n \treturn diff_flush_patch_id(options, sha1, diff_header_only);\n-- \n2.10.0.rc2.154.gb4a4b8b\n"},{"id":"301259","messageId":"alpine.DEB.2.20.1609071338310.129229@virtualbox","threadId":"44029","inReplyTo":"20160907075425.6jgamuwm66xbklu3@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] patch-ids: skip merge commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-07T12:52:04Z","receivedAt":"2016-09-07T12:52:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 7 Sep 2016, Jeff King wrote:\n\n> [...]\n> This patch just ignores merge commits entirely when\n> generating patch-ids, meaning they will never be matched\n> (from either side of a symmetric-diff traversal).\n\n... except it does not ignore merge commits:\n\n> diff --git a/patch-ids.c b/patch-ids.c\n> index 77e4663..b1f8514 100644\n> --- a/patch-ids.c\n> +++ b/patch-ids.c\n> @@ -7,10 +7,12 @@\n> int commit_patch_id(struct commit *commit, struct diff_options *options,\n> \t\t unsigned char *sha1, int diff_header_only)\n> {\n> -\tif (commit->parents)\n> +\tif (commit->parents) {\n> +\t\tif (commit->parents->next)\n> +\t\t\treturn 0;\n> \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n> \t\t\t commit->object.oid.hash, \"\", options);\n> -\telse\n> +\t} else\n\nWith this change, commit_patch_id() will return 0 for merge commits\n(indicating success) but it will not have touched the sha1! Which means it\nmay very well have all kinds of crap in the sha1 that may, or may not,\nmatch another, real patch ID randomly.\n\nSee e.g. the call site in builtin/log.c's prepare_bases():\n\n\t[...]\n\tif (commit_patch_id(commit, &diffopt, sha1, 0))\n\t\tdie(_(\"cannot get patch id\"));\n\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n\tpatch_id = bases->patch_id + bases->nr_patch_id;\n\thashcpy(patch_id->hash, sha1);\n\tbases->nr_patch_id++;\n\nSo the sha1 is actually used, even if it was not initialized.\n\nI would suggest to simply copy the merge commit's SHA-1. It is no patch\nID, of course, but collisions are as unlikely as commit name collisions,\nand it would make the \"patch ID\" of a merge commit deterministic again.\n\nI.e. something like\n\n \tif (commit->parents) {\n-\t\tif (commit->parents->next)\n+\t\tif (commit->parents->next) {\n+\t\t\thashcpy(sha1, commit->object.oid.hash);\n \t\t\treturn 0;\n+\t\t}\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n\non top.\n\nCiao,\nDscho\n"},{"id":"301260","messageId":"alpine.DEB.2.20.1609071452580.129229@virtualbox","threadId":"44029","inReplyTo":"20160907075411.cr67cbvmy7gxv2om@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] patch-ids: turn off rename detection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-07T12:53:15Z","receivedAt":"2016-09-07T12:53:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 7 Sep 2016, Jeff King wrote:\n\n> The patch-id code may be running inside another porcelain\n> like \"git log\" or \"git format-patch\", and therefore may have\n> set diff_detect_rename_default, either via the diff-ui\n> config, or by default since 5404c11 (diff: activate\n> diff.renames by default, 2016-02-25). This is the case even\n> if a command is run with `--no-renames`, as that is applied\n> only to the diff-options used by the command itself.\n> \n> Rename detection doesn't help the patch-id results. It\n> _may_ actually hurt, as minor differences in the files that\n> would be overlooked by patch-id's canonicalization might\n> result in different renames (though I'd doubt that it ever\n> comes up in practice).\n> \n> But mostly it is just a waste of CPU to compute these\n> renames.\n> \n> Note that we don't have to worry about compatibility here.\n> This patch disables renames just for the internal patch-id\n> comparison run by \"log --cherry-pick\", etc. The user-visible\n> \"git patch-id\" output depends on the patch that it is fed\n> (so it is up to the diff generator to use --no-renames if\n> they wish).\n\nSounds obviously good to me.\n\nCiao,\nDscho\n"},{"id":"301261","messageId":"alpine.DEB.2.20.1609071453240.129229@virtualbox","threadId":"44029","inReplyTo":"20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/2] more patch-id speedups","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-07T13:06:36Z","receivedAt":"2016-09-07T13:06:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 7 Sep 2016, Jeff King wrote:\n\n> Michael and I found a case where the \"format-patch --cherry-pick A...B\"\n> command for a rebase took over 7 minutes to run with git v2.9.3. Yikes.\n> \n> Switching to v2.10 dropped that to a bit over 3 minutes (due to the\n> kw/patch-ids-optim topic). Better, but not great.\n\nI agree: not great, but better...\n\n> The culprit turned out to be merge commits; the patch-id code will\n> happily diff a merge against its first parent, and ignore the rest. This\n> _seems_ like a bad idea, but maybe there is something clever going on\n> that I don't know about. I couldn't find anything useful in the history,\n> and given that this code was adapted from rebase, my guess is that it\n> was never really intended to handle merge commits in the first place (of\n> course we weren't trying to rebase merge commits; but it has to generate\n> patch-ids for everything that happened on \"A\" to compare against).\n> \n> Dropping the computation of the merge commits got it down to about 4\n> seconds. I also noticed that it was doing rename detection (which also\n> seems like a bad idea). Disabling renames dropped another half second or\n> so.\n\nThat makes for a really nice improvement!\n\n> This is marked as \"RFC\" because I don't feel entirely confident that I'm\n> not missing some clever need for these options. But in both cases my gut\n> feeling is that they are simply unintended effects that nobody ever\n> noticed, because it would be very rare that they would affect the\n> output. And that if they _did_ affect the output, they would probably be\n> doing the wrong thing.\n\nGiven that the patch ID is *wrong* for merge commits (it only looks at the\nfirst parent, so each \"-s ours\" merge will have the same patch ID!), I\nwould say that we can get away with re-defining the patch ID of merge\ncommits.\n\nThe only case where it might change things that I can think of would be a\n`git rebase --preserve-merges`: it would probably have worked *by chance*\nbefore (or not, in case of \"-s ours\" merges), and now it would try to pick\nthe merge commits even if rebased versions were already merged upstream.\n\nIf I read the --preserve-merges code correctly, that would result in the\nmerge commit's parents to be 'rewritten' to HEAD. And as both parents\nwould be rewritten to HEAD, they would be condensed into a single new\nparent, resulting in a cherry-pick that fails (because it tries to\ncherry-pick a merge commit without any -n option).\n\nOf course, what we could do is to introduce a modifier, e.g.\n--cherry-pick=first-parent, that would trigger the old behavior and would\nbe asked-for in the --preserve-merges mode.\n\nBut quite frankly, personally I would not worry about it *that* much. As\nyou pointed out, the patch ID for merge commits is incorrect to begin\nwith, and we may just redeclare all merge commits to be incomparable to\none another when it comes to patch IDs.\n\nIn short: I would be fine with the change of behavior.\n\nCiao,\nDscho\n"},{"id":"301311","messageId":"20160907184653.ed5c4oklyywjj2gp@sigill.intra.peff.net","threadId":"44029","inReplyTo":"alpine.DEB.2.20.1609071338310.129229@virtualbox","subject":"Re: [PATCH 2/2] patch-ids: skip merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T18:46:53Z","receivedAt":"2016-09-07T18:47:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 02:52:04PM +0200, Johannes Schindelin wrote:\n\n> > diff --git a/patch-ids.c b/patch-ids.c\n> > index 77e4663..b1f8514 100644\n> > --- a/patch-ids.c\n> > +++ b/patch-ids.c\n> > @@ -7,10 +7,12 @@\n> > int commit_patch_id(struct commit *commit, struct diff_options *options,\n> > \t\t unsigned char *sha1, int diff_header_only)\n> > {\n> > -\tif (commit->parents)\n> > +\tif (commit->parents) {\n> > +\t\tif (commit->parents->next)\n> > +\t\t\treturn 0;\n> > \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n> > \t\t\t commit->object.oid.hash, \"\", options);\n> > -\telse\n> > +\t} else\n> \n> With this change, commit_patch_id() will return 0 for merge commits\n> (indicating success) but it will not have touched the sha1! Which means it\n> may very well have all kinds of crap in the sha1 that may, or may not,\n> match another, real patch ID randomly.\n\nEek, thanks. Somehow I got it into my head that diff_flush_patch_id()\nbelow was what added it to the list, but clearly that is not the case.\nLooking at it again, I can't imagine how that is the case.\n\n> I would suggest to simply copy the merge commit's SHA-1. It is no patch\n> ID, of course, but collisions are as unlikely as commit name collisions,\n> and it would make the \"patch ID\" of a merge commit deterministic again.\n\nI agree that would work, though it does mean carrying extra useless\nentries in the patch_id hash. I'll see how bad it would be to simply\nomit them entirely, but this seems like a good fallback plan.\n\nThanks, and sorry for the obviously braindead patch.\n\n-Peff\n"},{"id":"301312","messageId":"20160907184911.sqwukfasnjjkr5gz@sigill.intra.peff.net","threadId":"44029","inReplyTo":"alpine.DEB.2.20.1609071453240.129229@virtualbox","subject":"Re: [RFC/PATCH 0/2] more patch-id speedups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T18:49:11Z","receivedAt":"2016-09-07T18:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 03:06:36PM +0200, Johannes Schindelin wrote:\n\n> > This is marked as \"RFC\" because I don't feel entirely confident that I'm\n> > not missing some clever need for these options. But in both cases my gut\n> > feeling is that they are simply unintended effects that nobody ever\n> > noticed, because it would be very rare that they would affect the\n> > output. And that if they _did_ affect the output, they would probably be\n> > doing the wrong thing.\n> \n> Given that the patch ID is *wrong* for merge commits (it only looks at the\n> first parent, so each \"-s ours\" merge will have the same patch ID!), I\n> would say that we can get away with re-defining the patch ID of merge\n> commits.\n> \n> The only case where it might change things that I can think of would be a\n> `git rebase --preserve-merges`: it would probably have worked *by chance*\n> before (or not, in case of \"-s ours\" merges), and now it would try to pick\n> the merge commits even if rebased versions were already merged upstream.\n> \n> If I read the --preserve-merges code correctly, that would result in the\n> merge commit's parents to be 'rewritten' to HEAD. And as both parents\n> would be rewritten to HEAD, they would be condensed into a single new\n> parent, resulting in a cherry-pick that fails (because it tries to\n> cherry-pick a merge commit without any -n option).\n> \n> Of course, what we could do is to introduce a modifier, e.g.\n> --cherry-pick=first-parent, that would trigger the old behavior and would\n> be asked-for in the --preserve-merges mode.\n> \n> But quite frankly, personally I would not worry about it *that* much. As\n> you pointed out, the patch ID for merge commits is incorrect to begin\n> with, and we may just redeclare all merge commits to be incomparable to\n> one another when it comes to patch IDs.\n> \n> In short: I would be fine with the change of behavior.\n\nThanks for this explanation; it matches what I was thinking, but you\nwent through it in a lot more detail.\n\nSo it sounds like this is the right thing, but as you pointed out, the\nimplementation is just silly. I'll see if I can come up with a working\nv2.\n\n-Peff\n"},{"id":"301328","messageId":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net","subject":"[RFC/PATCH v2 0/3] patch-id for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:01:01Z","receivedAt":"2016-09-07T22:01:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of the series I posted at:\n\n http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net/\n\nBasically, it drops the time for \"format-patch --cherry-pick\" on a\nparticular case from 3 minutes down to 3 seconds, by avoiding diffs\non merge commits. Compared to v1, it fixes the totally-broken handling\nof commit_patch_id() pointed out by Johannes.\n\nWe can drop the diffs on the merge commits because they're quite broken,\nas discussed in the commit message of patch 3 (they don't take into\naccount any parent except the first). So what do we do when somebody\nasks for the patch-id of a merge commit?\n\nThis is still marked RFC, because there are really two approaches here,\nand I'm not sure which one is better for \"format-patch --base\". I'd like\nto get input from Xiaolong Ye (who worked on --base), and Josh Triplett\n(who has proposed some patches in that area, and is presumably using\nthem).\n\nOption one is that merges are defined as having no patch-id at all. They\nare skipped for \"--cherry-pick\" comparison, and \"format-patch --base\"\nwill not mention them at all as prerequisites. That's what I've\nimplemented here.\n\nOption two is to use the commit sha1 as the patch-id for a merge, making\nit (essentially) unique. That gives us a defined value, but it's one\nthat \"--cherry-pick\" will not match between two segments of history. I\ndon't know if having _some_ defined value is useful for \"format-patch\n--base\" or not.\n\nAnd obviously there's an option 3: define some more complicated patch-id\nfor merges that takes into account all of the parents. I didn't think\ntoo much on that because I don't really see value in it over using the\ncommit sha1, and it would be computationally expensive.\n\n [1/3]: patch-ids: turn off rename detection\n [2/3]: diff_flush_patch_id: stop returning error result\n [3/3]: patch-ids: use commit sha1 as patch-id for merge commits\n\n-Peff\n"},{"id":"301329","messageId":"20160907220232.uotocvgdojfcl4o4@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","subject":"[PATCH 1/3] patch-ids: turn off rename detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:02:33Z","receivedAt":"2016-09-07T22:02:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-id code may be running inside another porcelain\nlike \"git log\" or \"git format-patch\", and therefore may have\nset diff_detect_rename_default, either via the diff-ui\nconfig, or by default since 5404c11 (diff: activate\ndiff.renames by default, 2016-02-25). This is the case even\nif a command is run with `--no-renames`, as that is applied\nonly to the diff-options used by the command itself.\n\nRename detection doesn't help the patch-id results. It\n_may_ actually hurt, as minor differences in the files that\nwould be overlooked by patch-id's canonicalization might\nresult in different renames (though I'd doubt that it ever\ncomes up in practice).\n\nBut mostly it is just a waste of CPU to compute these\nrenames.\n\nNote that this does have one user-visible impact: the\nprerequisite patches listed by \"format-patch --base\". There\nmay be some confusion between different versions of git as\nolder ones will enable renames, but newer ones will not.\nHowever, this was already a problem, as people with\ndifferent settings for the \"diff.renames\" config would get\ndifferent results. After this patch, everyone should get the\nsame results, regardless of their config.\n\nSigned-off-by: Jeff King \n---\nThe patch is the same as v1, but the commit message is modified, as I\nrealized that \"--base\" does expose this value publicly (but as I argue\nabove, this is probably an improvement).\n\n patch-ids.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 082412a..77e4663 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -45,6 +45,7 @@ int init_patch_ids(struct patch_ids *ids)\n {\n \tmemset(ids, 0, sizeof(*ids));\n \tdiff_setup(&ids->diffopts);\n+\tids->diffopts.detect_rename = 0;\n \tDIFF_OPT_SET(&ids->diffopts, RECURSIVE);\n \tdiff_setup_done(&ids->diffopts);\n \thashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301330","messageId":"20160907220409.oowxymhvkof2xsk5@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","subject":"[PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:04:09Z","receivedAt":"2016-09-07T22:04:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of our errors come from diff_get_patch_id(), which has\nexactly three error conditions. The first is an internal\nassertion, which should be a die(\"BUG\") in the first place.\n\nThe other two are caused by an inability to two diff blobs,\nwhich is an indication of a serious problem (probably\nrepository corruption). All the rest of the diff subsystem\ndies immediately on these conditions. By passing up the\nerror, in theory we can keep going even if patch-id is\nunable to function. But in practice this means we may\ngenerate subtly wrong results (e.g., by failing to correlate\ntwo commits). Let's just die(), as we're better off making\nit clear to the user that their repository is not\nfunctional.\n\nAs a result, we can simplify the calling code.\n\nSigned-off-by: Jeff King \n---\nThis is a prerequisite for patch 3, since it means that\ncommit_patch_id() stops returning \"real\" errors. But obviously if this\nis distasteful (and it does feel a little weird to convert error() to\ndie(), even though the rest of the diff code-base behaves this way), we\ncan teach commit_patch_id() to distinguish between \"this has no\npatch-id\" and \"a real error occured\" in its return value.\n\n diff.c | 18 ++++++++----------\n diff.h | 2 +-\n patch-ids.c | 3 ++-\n 3 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 534c12e..d0594f6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4462,7 +4462,7 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n }\n \n /* returns 0 upon success, and writes result into sha1 */\n-static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n+static void diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -4484,7 +4484,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\tif (p->status == 0)\n-\t\t\treturn error(\"internal diff status error\");\n+\t\t\tdie(\"BUG: diff status unset while computing patch_id\");\n \t\tif (p->status == DIFF_STATUS_UNKNOWN)\n \t\t\tcontinue;\n \t\tif (diff_unmodified_pair(p))\n@@ -4536,7 +4536,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \n \t\tif (fill_mmfile(&mf1, p->one) < 0 ||\n \t\t fill_mmfile(&mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\tdie(\"unable to read files to diff\");\n \n \t\tif (diff_filespec_is_binary(p->one) ||\n \t\t diff_filespec_is_binary(p->two)) {\n@@ -4552,27 +4552,25 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\txecfg.flags = 0;\n \t\tif (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\n \t\t\t\t &xpp, &xecfg))\n-\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n-\t\t\t\t p->one->path);\n+\t\t\tdie(\"unable to generate patch-id diff for %s\",\n+\t\t\t p->one->path);\n \t}\n \n \tgit_SHA1_Final(sha1, &ctx);\n-\treturn 0;\n }\n \n-int diff_flush_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n+void diff_flush_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n-\tint result = diff_get_patch_id(options, sha1, diff_header_only);\n+\n+\tdiff_get_patch_id(options, sha1, diff_header_only);\n \n \tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n \n \tfree(q->queue);\n \tDIFF_QUEUE_CLEAR(q);\n-\n-\treturn result;\n }\n \n static int is_summary_empty(const struct diff_queue_struct *q)\ndiff --git a/diff.h b/diff.h\nindex 7883729..f4dcfe1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -342,7 +342,7 @@ extern int run_diff_files(struct rev_info *revs, unsigned int option);\n extern int run_diff_index(struct rev_info *revs, int cached);\n \n extern int do_diff_cache(const unsigned char *, struct diff_options *);\n-extern int diff_flush_patch_id(struct diff_options *, unsigned char *, int);\n+extern void diff_flush_patch_id(struct diff_options *, unsigned char *, int);\n \n extern int diff_result_code(struct diff_options *, int);\n \ndiff --git a/patch-ids.c b/patch-ids.c\nindex 77e4663..0e95220 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -13,7 +13,8 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,\n \telse\n \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n \tdiffcore_std(options);\n-\treturn diff_flush_patch_id(options, sha1, diff_header_only);\n+\tdiff_flush_patch_id(options, sha1, diff_header_only);\n+\treturn 0;\n }\n \n /*\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301331","messageId":"20160907220439.ukbqnj5biaro2lxv@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","subject":"[PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:04:40Z","receivedAt":"2016-09-07T22:04:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-ids code which powers \"log --cherry-pick\" doesn't\nlook at whether each commit is a merge or not. It just feeds\nthe commit's first parent to the diff, and ignores any\nadditional parents.\n\nIn theory, this might be useful if you wanted to find\nequivalence between, say, a merge commit and a squash-merge\nthat does the same thing. But it also promotes a false\nequivalence between distinct merges; for example, every\n\"merge -s ours\" would look like an empty commit (which is\ntrue in a sense, but presumably there was a value in merging\nin the discarded history). Since patch-ids are meant for\nthrowing away duplicates, we should err on the side of _not_\nmatching such merges.\n\nMoreover, we may spend a lot of extra time computing these\nmerge diffs. In the case that inspired this patch, a \"git\nformat-patch --cherry-pick\" dropped from over 3 minutes to\nless than 4 seconds.\n\nThis seems pretty drastic, but is easily explained. The\ncommand was invoked by a \"git rebase\" of an older topic\nbranch; there had been tens of thousands of commits on the\nupstream branch in the meantime. In addition, this project\nused a topic-branch workflow with occasional \"back-merges\"\nfrom \"master\" to each topic (to resolve conflicts on the\ntopics rather than in the merge commits). So there were not\nonly extra merges, but the diffs for these back-merges were\ngenerally quite large (because they represented _everything_\nthat had been merged to master since the topic branched).\n\nThis patch defines the patch-id of a merge commit as\nessentially \"null\"; it has no patch-id. As a result,\nmerges cannot match patch-ids via \"--cherry-pick\", and\n\"format-patch --base\" will not list merges in its list of\nprerequisite patch ids.\n\nWe can signal the null patch-id by returning \"-1\" from\ncommit_patch_id(), as it no longer returns any meaningful\nerrors (and as a result, its callers are updated to handle\n\"-1\" differently).\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\n builtin/log.c | 2 +-\n patch-ids.c | 14 +++++++-------\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 92dc34d..e660034 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1344,7 +1344,7 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\tif (commit->util)\n \t\t\tcontinue;\n \t\tif (commit_patch_id(commit, &diffopt, sha1, 0))\n-\t\t\tdie(_(\"cannot get patch id\"));\n+\t\t\tcontinue;\n \t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n \t\tpatch_id = bases->patch_id + bases->nr_patch_id;\n \t\thashcpy(patch_id->hash, sha1);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 0e95220..197c70b 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -7,10 +7,12 @@\n int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t unsigned char *sha1, int diff_header_only)\n {\n-\tif (commit->parents)\n+\tif (commit->parents) {\n+\t\tif (commit->parents->next)\n+\t\t\treturn -1;\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n \t\t\t commit->object.oid.hash, \"\", options);\n-\telse\n+\t} else\n \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n \tdiffcore_std(options);\n \tdiff_flush_patch_id(options, sha1, diff_header_only);\n@@ -19,7 +21,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,\n \n /*\n * When we cannot load the full patch-id for both commits for whatever\n- * reason, the function returns -1 (i.e. return error(...)). Despite\n+ * reason, the function returns -1. Despite\n * the \"cmp\" in the name of this function, the caller only cares about\n * the return value being zero (a and b are equivalent) or non-zero (a\n * and b are different), and returning non-zero would keep both in the\n@@ -33,12 +35,10 @@ static int patch_id_cmp(struct patch_id *a,\n {\n \tif (is_null_sha1(a->patch_id) &&\n \t commit_patch_id(a->commit, opt, a->patch_id, 0))\n-\t\treturn error(\"Could not get patch ID for %s\",\n-\t\t\toid_to_hex(&a->commit->object.oid));\n+\t\treturn -1;\n \tif (is_null_sha1(b->patch_id) &&\n \t commit_patch_id(b->commit, opt, b->patch_id, 0))\n-\t\treturn error(\"Could not get patch ID for %s\",\n-\t\t\toid_to_hex(&b->commit->object.oid));\n+\t\treturn -1;\n \treturn hashcmp(a->patch_id, b->patch_id);\n }\n \n-- \n2.10.0.rc2.154.gb4a4b8b\n"},{"id":"301332","messageId":"20160907220803.3cvjkchpmyff7h5f@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907184653.ed5c4oklyywjj2gp@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] patch-ids: skip merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:08:03Z","receivedAt":"2016-09-07T22:08:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 02:46:53PM -0400, Jeff King wrote:\n\n> > With this change, commit_patch_id() will return 0 for merge commits\n> > (indicating success) but it will not have touched the sha1! Which means it\n> > may very well have all kinds of crap in the sha1 that may, or may not,\n> > match another, real patch ID randomly.\n> \n> Eek, thanks. Somehow I got it into my head that diff_flush_patch_id()\n> below was what added it to the list, but clearly that is not the case.\n> Looking at it again, I can't imagine how that is the case.\n\nAh, I see. I initially was looking at an older git (v2.6.x), in which\ncommit_patch_id() is a static function inside patch-ids.c, and we do not\ndo any lazy-load trickery. But note that the patch is still wrong even\nthere; it should return \"-1\" from commit_patch_id() to instruct the\ncaller not to add it to the hash.\n\nAnyway...\n\n> > I would suggest to simply copy the merge commit's SHA-1. It is no patch\n> > ID, of course, but collisions are as unlikely as commit name collisions,\n> > and it would make the \"patch ID\" of a merge commit deterministic again.\n> \n> I agree that would work, though it does mean carrying extra useless\n> entries in the patch_id hash. I'll see how bad it would be to simply\n> omit them entirely, but this seems like a good fallback plan.\n\nIt's not too hard to do so, but it raises a question of what\n\"format-patch --base\" would want. I've just sent another RFC cc-ing\nfolks interested in that area.\n\nThanks again for the review.\n\n-Peff\n"},{"id":"301333","messageId":"CA+P7+xriHk=UrBhLD1mWK7mWBth_2du7jgLPSAkoSAjKz0de-Q@mail.gmail.com","threadId":"44029","inReplyTo":"20160907220232.uotocvgdojfcl4o4@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] patch-ids: turn off rename detection","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-09-07T22:12:17Z","receivedAt":"2016-09-07T22:12:59Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 7, 2016 at 3:02 PM, Jeff King wrote:\n> The patch-id code may be running inside another porcelain\n> like \"git log\" or \"git format-patch\", and therefore may have\n> set diff_detect_rename_default, either via the diff-ui\n> config, or by default since 5404c11 (diff: activate\n> diff.renames by default, 2016-02-25). This is the case even\n> if a command is run with `--no-renames`, as that is applied\n> only to the diff-options used by the command itself.\n>\n> Rename detection doesn't help the patch-id results. It\n> _may_ actually hurt, as minor differences in the files that\n> would be overlooked by patch-id's canonicalization might\n> result in different renames (though I'd doubt that it ever\n> comes up in practice).\n>\n> But mostly it is just a waste of CPU to compute these\n> renames.\n\nYes this seems reasonable.\n\n>\n> Note that this does have one user-visible impact: the\n> prerequisite patches listed by \"format-patch --base\". There\n> may be some confusion between different versions of git as\n> older ones will enable renames, but newer ones will not.\n> However, this was already a problem, as people with\n> different settings for the \"diff.renames\" config would get\n> different results. After this patch, everyone should get the\n> same results, regardless of their config.\n\nMakes sense.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> The patch is the same as v1, but the commit message is modified, as I\n> realized that \"--base\" does expose this value publicly (but as I argue\n> above, this is probably an improvement).\n\nI agree this is an improvement.\n\nThanks,\nJake\n"},{"id":"301335","messageId":"CA+P7+xr_GqU+v6RevEuqgNK5Kui=izRwD2oPvBh-4NzAp10uOQ@mail.gmail.com","threadId":"44029","inReplyTo":"20160907220439.ukbqnj5biaro2lxv@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-09-07T22:28:10Z","receivedAt":"2016-09-07T22:28:37Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 7, 2016 at 3:04 PM, Jeff King wrote:\n> The patch-ids code which powers \"log --cherry-pick\" doesn't\n> look at whether each commit is a merge or not. It just feeds\n> the commit's first parent to the diff, and ignores any\n> additional parents.\n>\n\nThe subject here is misleading since it says you will use sha1 of the\nmerge commit ,but instead just ignore merge commits and indicate they\nhave no patch id. I suspect this is because you switched\nimplementations part way through developing this.\n\nThanks,\nJake\n"},{"id":"301336","messageId":"20160907223816.jio6gk72uuknq4ha@sigill.intra.peff.net","threadId":"44029","inReplyTo":"CA+P7+xr_GqU+v6RevEuqgNK5Kui=izRwD2oPvBh-4NzAp10uOQ@mail.gmail.com","subject":"Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T22:38:17Z","receivedAt":"2016-09-07T22:38:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 03:28:10PM -0700, Jacob Keller wrote:\n\n> On Wed, Sep 7, 2016 at 3:04 PM, Jeff King wrote:\n> > The patch-ids code which powers \"log --cherry-pick\" doesn't\n> > look at whether each commit is a merge or not. It just feeds\n> > the commit's first parent to the diff, and ignores any\n> > additional parents.\n> >\n> \n> The subject here is misleading since it says you will use sha1 of the\n> merge commit ,but instead just ignore merge commits and indicate they\n> have no patch id. I suspect this is because you switched\n> implementations part way through developing this.\n\nOops, yes, that's exactly what happened. I'll wait for discussion to\nsettle on the approach and fix it up in the re-roll.\n\n-Peff\n"},{"id":"301337","messageId":"20160907225104.f5wi2yo4d2f26tti@x","threadId":"44029","inReplyTo":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","subject":"Re: [RFC/PATCH v2 0/3] patch-id for merges","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-07T22:51:04Z","receivedAt":"2016-09-07T22:51:19Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Wed, Sep 07, 2016 at 06:01:01PM -0400, Jeff King wrote:\n> Here's a re-roll of the series I posted at:\n> \n> http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net/\n> \n> Basically, it drops the time for \"format-patch --cherry-pick\" on a\n> particular case from 3 minutes down to 3 seconds, by avoiding diffs\n> on merge commits. Compared to v1, it fixes the totally-broken handling\n> of commit_patch_id() pointed out by Johannes.\n> \n> We can drop the diffs on the merge commits because they're quite broken,\n> as discussed in the commit message of patch 3 (they don't take into\n> account any parent except the first). So what do we do when somebody\n> asks for the patch-id of a merge commit?\n> \n> This is still marked RFC, because there are really two approaches here,\n> and I'm not sure which one is better for \"format-patch --base\". I'd like\n> to get input from Xiaolong Ye (who worked on --base), and Josh Triplett\n> (who has proposed some patches in that area, and is presumably using\n> them).\n\nThanks.\n\nI'd love to see a more resilient patch-id mechanism, to make it easier\nto match up patches between branches. I don't think it makes sense to\ntalk about the patch-id of a merge commit (though it might make sense\nfor a merge which makes additional changes not present in any of the\nparents). Even if someone wants to match up merge commits with merge\ncommits, I don't think that should happen via patch-id; I think that\nshould happen in terms of \"what patches does this merge introduce\",\nwithout constructing a merge-patch-id via a Merkle tree of commit\npatch-ids.\n\nSo, I think this patch series makes sense (modulo the comments about the\ncommit message in patch 3). We already don't respect merge commits when\ndoing format-patch; this seems consistent with that. If we ever make it\npossible for format-patch to handle merge commits, then we should also\nallow it to have merge commits as prerequisites.\n\n- Josh Triplett\n"},{"id":"301351","messageId":"5c4276ad-19ed-1481-41cf-3d506ef9c7c6@ramsayjones.plus.com","threadId":"44029","inReplyTo":"20160907220409.oowxymhvkof2xsk5@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-09-08T00:51:05Z","receivedAt":"2016-09-08T00:51:16Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 07/09/16 23:04, Jeff King wrote:\n> All of our errors come from diff_get_patch_id(), which has\n> exactly three error conditions. The first is an internal\n> assertion, which should be a die(\"BUG\") in the first place.\n> \n> The other two are caused by an inability to two diff blobs,\n ^^^^^^^^^^^^^^^^^\nHuh? ... to diff two blobs?\n\nATB,\nRamsay Jones\n"},{"id":"301372","messageId":"20160908072029.rwsa5b6zgwwe37ly@sigill.intra.peff.net","threadId":"44029","inReplyTo":"5c4276ad-19ed-1481-41cf-3d506ef9c7c6@ramsayjones.plus.com","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T07:20:29Z","receivedAt":"2016-09-08T07:20:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 01:51:05AM +0100, Ramsay Jones wrote:\n\n> \n> \n> On 07/09/16 23:04, Jeff King wrote:\n> > All of our errors come from diff_get_patch_id(), which has\n> > exactly three error conditions. The first is an internal\n> > assertion, which should be a die(\"BUG\") in the first place.\n> > \n> > The other two are caused by an inability to two diff blobs,\n> ^^^^^^^^^^^^^^^^^\n> Huh? ... to diff two blobs?\n\nSorry. English my getting worse be to seems.\n\nWill fix in a re-roll.\n\n-Peff\n"},{"id":"301373","messageId":"20160908073019.jqgocmvjuzwixvdk@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907225104.f5wi2yo4d2f26tti@x","subject":"Re: [RFC/PATCH v2 0/3] patch-id for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T07:30:19Z","receivedAt":"2016-09-08T07:30:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 03:51:04PM -0700, Josh Triplett wrote:\n\n> > This is still marked RFC, because there are really two approaches here,\n> > and I'm not sure which one is better for \"format-patch --base\". I'd like\n> > to get input from Xiaolong Ye (who worked on --base), and Josh Triplett\n> > (who has proposed some patches in that area, and is presumably using\n> > them).\n> \n> Thanks.\n> \n> I'd love to see a more resilient patch-id mechanism, to make it easier\n> to match up patches between branches. I don't think it makes sense to\n> talk about the patch-id of a merge commit (though it might make sense\n> for a merge which makes additional changes not present in any of the\n> parents). Even if someone wants to match up merge commits with merge\n> commits, I don't think that should happen via patch-id; I think that\n> should happen in terms of \"what patches does this merge introduce\",\n> without constructing a merge-patch-id via a Merkle tree of commit\n> patch-ids.\n> \n> So, I think this patch series makes sense (modulo the comments about the\n> commit message in patch 3). We already don't respect merge commits when\n> doing format-patch; this seems consistent with that. If we ever make it\n> possible for format-patch to handle merge commits, then we should also\n> allow it to have merge commits as prerequisites.\n\nThanks for the input. I knew that format-patch doesn't show merge\ncommits, but I didn't realize that merges were skipped entirely for the\nbase preparation (but I see it now; there is a \"rev.max_parents = 1\"\nsetting in prepare_bases).\n\nSo this really doesn't change the output there at all. And in fact, the\nswitch to:\n\n if (commit_patch_id(commit, &diffopt, sha1, 0))\n - die(_(\"cannot get patch id\"))\n + continue;\n\nshould never hit that continue. It could be:\n\n die(\"BUG: somehow a merge got fed to commit_patch_id?\");\n\nbut the \"continue\" somehow seems like the right thing to me.\n\nI'll wait another day or so for comments and then send the re-roll using\nthis approach.\n\n-Peff\n"},{"id":"301476","messageId":"alpine.DEB.2.20.1609091219350.129229@virtualbox","threadId":"44029","inReplyTo":"20160907220409.oowxymhvkof2xsk5@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T10:28:38Z","receivedAt":"2016-09-09T10:29:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 7 Sep 2016, Jeff King wrote:\n\n> All of our errors come from diff_get_patch_id(), which has\n> exactly three error conditions. The first is an internal\n> assertion, which should be a die(\"BUG\") in the first place.\n> \n> The other two are caused by an inability to two diff blobs,\n> which is an indication of a serious problem (probably\n> repository corruption). All the rest of the diff subsystem\n> dies immediately on these conditions. By passing up the\n> error, in theory we can keep going even if patch-id is\n> unable to function. But in practice this means we may\n> generate subtly wrong results (e.g., by failing to correlate\n> two commits). Let's just die(), as we're better off making\n> it clear to the user that their repository is not\n> functional.\n> \n> As a result, we can simplify the calling code.\n\nI like the simplification, but I *hate* the fact that the calling code has\n*no way* to inform the user about the proper next steps.\n\nYou are touching code that is really quite at the bottom of a lot of call\nchains. For example in the one of `git pull --rebase`. I just spent an\ninsane amount of time trying to make sure that this command will not\nsimply die() somewhere deep in the code, leaving the user puzzled.\n\nPlease see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful\nadvice when failing, 2016-07-26) for more details.\n\nA much better way, in my opinion, would be to introduce a new flag, say,\nskip_merges, and pass that to the diff_flush_patch_id() function. You\ncould also consider consolidating that flag with the diff_header_only flag\ninto a \"flags\" argument via something like\n\n\tenum diff_flush_patch_id {\n\t\tDIFF_HEADER_ONLY = 1,\n\t\tSKIP_MERGES = 2\n\t}\n\nBut it is definitely not a good idea to reintroduce the bad practice of\ndie()ing deep down in library code. I know, you want proper exception\nhandling. We cannot have that. We use C. But die() is not a solution: it\nintroduces new problems.\n\nMind you: I agree that there are serious problems in the cases you\nillustrated. But none of those problems give us license to leave the user\nutterly puzzled by not even telling them what is going on: spouting\ninternals such as \"unable to read files to diff\" is *most definitely* not\nhelping users who simply want to run a `git pull --rebase`.\n\nCiao,\nDscho\n"},{"id":"301480","messageId":"20160909104007.pwki2ir6et3vvk55@sigill.intra.peff.net","threadId":"44029","inReplyTo":"alpine.DEB.2.20.1609091219350.129229@virtualbox","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T10:40:08Z","receivedAt":"2016-09-09T10:40:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote:\n\n> I like the simplification, but I *hate* the fact that the calling code has\n> *no way* to inform the user about the proper next steps.\n> \n> You are touching code that is really quite at the bottom of a lot of call\n> chains. For example in the one of `git pull --rebase`. I just spent an\n> insane amount of time trying to make sure that this command will not\n> simply die() somewhere deep in the code, leaving the user puzzled.\n> \n> Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful\n> advice when failing, 2016-07-26) for more details.\n\nYes, I agree that this is the opposite direction of libification. And I\nagree that the current message is not very helpful.\n\nBut I am not sure that returning the error up the stack will actually\nhelp somebody move forward. The reason these are all die() calls in the\nrest of the diff code is that they are generally indicative of\nunrecoverable repository corruption. So any advice does not really\ndepend on what operation you are performing; it is always \"stop what you\nare doing immediately, run fsck, and try to get the broken objects from\nsomebody else\".\n\nSo IMHO, on balance this is not hurting anything.\n\n> A much better way, in my opinion, would be to introduce a new flag, say,\n> skip_merges, and pass that to the diff_flush_patch_id() function. You\n> could also consider consolidating that flag with the diff_header_only flag\n> into a \"flags\" argument via something like\n\ndiff_flush_patch_id() doesn't care about merges; that's too late. The\nchange has to happen in commit_patch_id(). And the problem is not one of\npassing in \"skip merges\" (we _always_ want to skip merges). It is rather\ndistinguishing the reason that commit_patch_id() told us it did not fill\nin the sha1: because it was an error, or because the patch id is\nundefined (one triggers a die(), the other a silent continue).\n\nI think I laid out that path already in the cover letter of the\noriginal. If the consensus is that this is too ugly, I can implement\nthat approach.\n\n-Peff\n"},{"id":"301488","messageId":"alpine.DEB.2.20.1609091455180.129229@virtualbox","threadId":"44029","inReplyTo":"20160909104007.pwki2ir6et3vvk55@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T12:58:25Z","receivedAt":"2016-09-09T12:59:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Sep 2016, Jeff King wrote:\n\n> On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote:\n> \n> > I like the simplification, but I *hate* the fact that the calling code has\n> > *no way* to inform the user about the proper next steps.\n> > \n> > You are touching code that is really quite at the bottom of a lot of call\n> > chains. For example in the one of `git pull --rebase`. I just spent an\n> > insane amount of time trying to make sure that this command will not\n> > simply die() somewhere deep in the code, leaving the user puzzled.\n> > \n> > Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful\n> > advice when failing, 2016-07-26) for more details.\n> \n> Yes, I agree that this is the opposite direction of libification. And I\n> agree that the current message is not very helpful.\n> \n> But I am not sure that returning the error up the stack will actually\n> help somebody move forward. The reason these are all die() calls in the\n> rest of the diff code is that they are generally indicative of\n> unrecoverable repository corruption. So any advice does not really\n> depend on what operation you are performing; it is always \"stop what you\n> are doing immediately, run fsck, and try to get the broken objects from\n> somebody else\".\n> \n> So IMHO, on balance this is not hurting anything.\n\nWell, you make such a situation even worse than it already is.\n\nIt would be one thing to change the code to actually say \"stop what you\nare doing immediately, run `git fsck` and try to get the broken objects\nfrom somewhere else\", *before* saying how to proceed after that.\n\nBut that is not what your patch does.\n\nWhat your patch does is to remove *even the possibility* of saying how to\nproceed after getting the repository corruption fixed. And instead of\nsaying how the corruption could be fixed, it outputs a terse \"cannot read\nfiles to diff\".\n\nI do not think that is a wise direction.\n\nCiao,\nDscho\n"},{"id":"301547","messageId":"20160909193756.okvgzjiqcr5jo6hf@sigill.intra.peff.net","threadId":"44029","inReplyTo":"alpine.DEB.2.20.1609091455180.129229@virtualbox","subject":"Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T19:37:56Z","receivedAt":"2016-09-09T19:38:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 02:58:25PM +0200, Johannes Schindelin wrote:\n\n> > Yes, I agree that this is the opposite direction of libification. And I\n> > agree that the current message is not very helpful.\n> > \n> > But I am not sure that returning the error up the stack will actually\n> > help somebody move forward. The reason these are all die() calls in the\n> > rest of the diff code is that they are generally indicative of\n> > unrecoverable repository corruption. So any advice does not really\n> > depend on what operation you are performing; it is always \"stop what you\n> > are doing immediately, run fsck, and try to get the broken objects from\n> > somebody else\".\n> > \n> > So IMHO, on balance this is not hurting anything.\n> \n> Well, you make such a situation even worse than it already is.\n> \n> It would be one thing to change the code to actually say \"stop what you\n> are doing immediately, run `git fsck` and try to get the broken objects\n> from somewhere else\", *before* saying how to proceed after that.\n> \n> But that is not what your patch does.\n> \n> What your patch does is to remove *even the possibility* of saying how to\n> proceed after getting the repository corruption fixed. And instead of\n> saying how the corruption could be fixed, it outputs a terse \"cannot read\n> files to diff\".\n> \n> I do not think that is a wise direction.\n\nFirst, do not blame me for the terse \"cannot read files to diff\". That\nis the current message. And my patch does not make changing that message\nany more difficult. You are welcome to change it in its error() form.\nYou are welcome to change it in the resulting die().\n\nThe quality of that message is totally orthogonal to what the patch is\ndoing.\n\nThe _only_ thing it is losing is the ability to for the caller to then\nadditionally say \"once you have finished uncorrupting the repository,\nyou can resume your operation with ...\".\n\nMy point is that this is not useful advice. No callers give it, and I\ndon't foresee other callers giving it. My argument above was basically\nthat it is such an exceptional condition it is not worth worrying about.\n\n-Peff\n"},{"id":"301557","messageId":"20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net","subject":"[PATCH v3 0/2] patch-id for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T20:34:06Z","receivedAt":"2016-09-09T20:34:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 06:01:01PM -0400, Jeff King wrote:\n\n> Here's a re-roll of the series I posted at:\n> \n> http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net/\n> \n> Basically, it drops the time for \"format-patch --cherry-pick\" on a\n> particular case from 3 minutes down to 3 seconds, by avoiding diffs\n> on merge commits. Compared to v1, it fixes the totally-broken handling\n> of commit_patch_id() pointed out by Johannes.\n>\n> [1/3]: patch-ids: turn off rename detection\n> [2/3]: diff_flush_patch_id: stop returning error result\n> [3/3]: patch-ids: use commit sha1 as patch-id for merge commits\n\nAnd here is v3. Besides commit-message fixups, it drops patch 2, and\ninstead the third patch teaches commit_patch_id() to distinguish between\nerrors and \"no patch id\".\n\nFrankly, I still like v2 better, but I do not feel like arguing with\nJohannes about it anymore.\n\n [1/2]: patch-ids: turn off rename detection\n [2/2]: patch-ids: define patch-id of merge commits as \"null\"\n\n-Peff\n"},{"id":"301558","messageId":"20160909203434.lkwhkaouk2ckwi6z@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net","subject":"[PATCH v3 1/2] patch-ids: turn off rename detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T20:34:34Z","receivedAt":"2016-09-09T20:34:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-id code may be running inside another porcelain\nlike \"git log\" or \"git format-patch\", and therefore may have\nset diff_detect_rename_default, either via the diff-ui\nconfig, or by default since 5404c11 (diff: activate\ndiff.renames by default, 2016-02-25). This is the case even\nif a command is run with `--no-renames`, as that is applied\nonly to the diff-options used by the command itself.\n\nRename detection doesn't help the patch-id results. It\n_may_ actually hurt, as minor differences in the files that\nwould be overlooked by patch-id's canonicalization might\nresult in different renames (though I'd doubt that it ever\ncomes up in practice).\n\nBut mostly it is just a waste of CPU to compute these\nrenames.\n\nNote that this does have one user-visible impact: the\nprerequisite patches listed by \"format-patch --base\". There\nmay be some confusion between different versions of git as\nolder ones will enable renames, but newer ones will not.\nHowever, this was already a problem, as people with\ndifferent settings for the \"diff.renames\" config would get\ndifferent results. After this patch, everyone should get the\nsame results, regardless of their config.\n\nSigned-off-by: Jeff King \n---\n patch-ids.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 082412a..77e4663 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -45,6 +45,7 @@ int init_patch_ids(struct patch_ids *ids)\n {\n \tmemset(ids, 0, sizeof(*ids));\n \tdiff_setup(&ids->diffopts);\n+\tids->diffopts.detect_rename = 0;\n \tDIFF_OPT_SET(&ids->diffopts, RECURSIVE);\n \tdiff_setup_done(&ids->diffopts);\n \thashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);\n-- \n2.10.0.161.gdb62534\n\n"},{"id":"301559","messageId":"20160909203447.nlyx6v76fpyx2vzl@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net","subject":"[PATCH v3 2/2] patch-ids: define patch-id of merge commits as \"null\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T20:34:47Z","receivedAt":"2016-09-09T20:34:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patch-id code which powers \"log --cherry-pick\" doesn't\nlook at whether each commit is a merge or not. It just feeds\nthe commit's first parent to the diff, and ignores any\nadditional parents.\n\nIn theory, this might be useful if you wanted to find\nequivalence between, say, a merge commit and a squash-merge\nthat does the same thing. But it also promotes a false\nequivalence between distinct merges. For example, every\n\"merge -s ours\" would look like identical to an empty commit\n(which is true in a sense, but presumably there was a value\nin merging in the discarded history). Since patch-ids are\nmeant for throwing away duplicates, we should err on the\nside of _not_ matching such merges.\n\nMoreover, we may spend a lot of extra time computing these\nmerge diffs. In the case that inspired this patch, a \"git\nformat-patch --cherry-pick\" dropped from over 3 minutes to\nless than 4 seconds.\n\nThis seems pretty drastic, but is easily explained. The\ncommand was invoked by a \"git rebase\" of an older topic\nbranch; there had been tens of thousands of commits on the\nupstream branch in the meantime. In addition, this project\nused a topic-branch workflow with occasional \"back-merges\"\nfrom \"master\" to each topic (to resolve conflicts on the\ntopics rather than in the merge commits). So there were not\nonly extra merges, but the diffs for these back-merges were\ngenerally quite large (because they represented _everything_\nthat had been merged to master since the topic branched).\n\nThis patch defines the patch-id of a merge commit as\nessentially \"null\"; it has no patch-id. As a result,\nmerges cannot match patch-ids via \"--cherry-pick\", and\n\"format-patch --base\" will not list merges in its list of\nprerequisite patch ids.\n\nTo distinguish between real errors and \"null\", we have to\nexpand the semantics of commit_patch_id()'s return value,\nand callers need to distinguish these cases.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\n builtin/log.c | 10 +++++++++-\n patch-ids.c | 40 ++++++++++++++++++++++++++++------------\n patch-ids.h | 11 +++++++++--\n 3 files changed, 46 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 92dc34d..ced1ea7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1343,8 +1343,16 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\tstruct object_id *patch_id;\n \t\tif (commit->util)\n \t\t\tcontinue;\n-\t\tif (commit_patch_id(commit, &diffopt, sha1, 0))\n+\n+\t\tswitch (commit_patch_id(commit, &diffopt, sha1, 0)) {\n+\t\tcase PATCH_ID_OK:\n+\t\t\tbreak;\n+\t\tcase PATCH_ID_NONE:\n+\t\t\tcontinue;\n+\t\tcase PATCH_ID_ERROR:\n \t\t\tdie(_(\"cannot get patch id\"));\n+\t\t}\n+\n \t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n \t\tpatch_id = bases->patch_id + bases->nr_patch_id;\n \t\thashcpy(patch_id->hash, sha1);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 77e4663..8d06099 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -7,18 +7,40 @@\n int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t unsigned char *sha1, int diff_header_only)\n {\n-\tif (commit->parents)\n+\tif (commit->parents) {\n+\t\tif (commit->parents->next)\n+\t\t\treturn PATCH_ID_NONE;\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n \t\t\t commit->object.oid.hash, \"\", options);\n-\telse\n+\t} else\n \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n \tdiffcore_std(options);\n-\treturn diff_flush_patch_id(options, sha1, diff_header_only);\n+\tif (diff_flush_patch_id(options, sha1, diff_header_only))\n+\t\treturn PATCH_ID_ERROR;\n+\treturn PATCH_ID_OK;\n+}\n+\n+/* avoid repeating ourselves in patch_id_cmp */\n+static int cmp_setup(struct patch_id *p, struct diff_options *opt)\n+{\n+\tif (!is_null_sha1(p->patch_id))\n+\t\treturn 0; /* OK, already computed id */\n+\n+\tswitch (commit_patch_id(p->commit, opt, p->patch_id, 0)) {\n+\tcase PATCH_ID_OK:\n+\t\treturn 0;\n+\tcase PATCH_ID_ERROR:\n+\t\treturn error(\"Could not get patch ID for %s\",\n+\t\t\t oid_to_hex(&p->commit->object.oid));\n+\tcase PATCH_ID_NONE:\n+\t\treturn -1; /* not an error, but nothing to compare */\n+\t}\n+\tdie(\"BUG: unhandled patch_result\");\n }\n \n /*\n * When we cannot load the full patch-id for both commits for whatever\n- * reason, the function returns -1 (i.e. return error(...)). Despite\n+ * reason, the function returns -1. Despite\n * the \"cmp\" in the name of this function, the caller only cares about\n * the return value being zero (a and b are equivalent) or non-zero (a\n * and b are different), and returning non-zero would keep both in the\n@@ -30,14 +52,8 @@ static int patch_id_cmp(struct patch_id *a,\n \t\t\tstruct patch_id *b,\n \t\t\tstruct diff_options *opt)\n {\n-\tif (is_null_sha1(a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, a->patch_id, 0))\n-\t\treturn error(\"Could not get patch ID for %s\",\n-\t\t\toid_to_hex(&a->commit->object.oid));\n-\tif (is_null_sha1(b->patch_id) &&\n-\t commit_patch_id(b->commit, opt, b->patch_id, 0))\n-\t\treturn error(\"Could not get patch ID for %s\",\n-\t\t\toid_to_hex(&b->commit->object.oid));\n+\tif (cmp_setup(a, opt) || cmp_setup(b, opt))\n+\t\treturn -1;\n \treturn hashcmp(a->patch_id, b->patch_id);\n }\n \ndiff --git a/patch-ids.h b/patch-ids.h\nindex 0f34ea1..96fd2b9 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -12,8 +12,15 @@ struct patch_ids {\n \tstruct diff_options diffopts;\n };\n \n-int commit_patch_id(struct commit *commit, struct diff_options *options,\n-\t\t unsigned char *sha1, int);\n+enum patch_id_result {\n+\tPATCH_ID_ERROR = -1,\n+\tPATCH_ID_OK = 0,\n+\tPATCH_ID_NONE\n+};\n+\n+enum patch_id_result commit_patch_id(struct commit *commit,\n+\t\t\t\t struct diff_options *options,\n+\t\t\t\t unsigned char *sha1, int);\n int init_patch_ids(struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);\n-- \n2.10.0.161.gdb62534\n"},{"id":"301560","messageId":"20160909203742.cyg3y7ju6vhfrx7o@sigill.intra.peff.net","threadId":"44029","inReplyTo":"20160909203447.nlyx6v76fpyx2vzl@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] patch-ids: define patch-id of merge commits as \"null\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T20:37:42Z","receivedAt":"2016-09-09T20:37:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 04:34:47PM -0400, Jeff King wrote:\n\n> This patch defines the patch-id of a merge commit as\n> essentially \"null\"; it has no patch-id. As a result,\n> merges cannot match patch-ids via \"--cherry-pick\", and\n> \"format-patch --base\" will not list merges in its list of\n> prerequisite patch ids.\n> \n> To distinguish between real errors and \"null\", we have to\n> expand the semantics of commit_patch_id()'s return value,\n> and callers need to distinguish these cases.\n\nOne alternative would be to add an out-parameter that is set in the\nsuccess case saying \"yes, we have a real patch-id\". And then the callers\ncould look like:\n\n if (commit_patch_id(commit, &diffopt, sha1, 0, &got_one))\n\tdie(\"error!\");\n if (!got_one)\n\tcontinue; /* silently skip */\n\nWe could even use the null sha1 to signal that rather than an extra\nparameter, I suppose.\n\nI dunno. It would make the callers less clunky, I think, but it does\nfeel a bit magical.\n\n-Peff\n"},{"id":"301566","messageId":"xmqqfup8aiud.fsf@gitster.mtv.corp.google.com","threadId":"44029","inReplyTo":"20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T21:01:14Z","receivedAt":"2016-09-09T21:01:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> And here is v3. Besides commit-message fixups, it drops patch 2, and\n> instead the third patch teaches commit_patch_id() to distinguish between\n> errors and \"no patch id\".\n>\n> Frankly, I still like v2 better, but I do not feel like arguing with\n> Johannes about it anymore.\n\nFWIW, I too like the simplicity of v2, as all the error-to-die\nconversion is for cases in which there is no sane recovery path.\n\nI'll have to take a bit deeper look at [v3 2/2] that had to become\nmore involved to decide if the additional flexibility is really\nworth it.\n\nThanks.\n\n"},{"id":"301568","messageId":"xmqqbmzwaia1.fsf@gitster.mtv.corp.google.com","threadId":"44029","inReplyTo":"20160909203447.nlyx6v76fpyx2vzl@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] patch-ids: define patch-id of merge commits as \"null\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T21:13:26Z","receivedAt":"2016-09-09T21:13:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This patch defines the patch-id of a merge commit as\n> essentially \"null\"; it has no patch-id. As a result,\n> merges cannot match patch-ids via \"--cherry-pick\", and\n> \"format-patch --base\" will not list merges in its list of\n> prerequisite patch ids.\n\nAt first I wondered if such a change would make all merges look the\nsame, but the patch-ids.c comparison is not for ordering/sorting but\nonly for equality, so as long as the comparison function knows that\na comparison of anything with \"null\" yields \"They are different\", we\nare OK.\n\n> diff --git a/patch-ids.c b/patch-ids.c\n> index 77e4663..8d06099 100644\n> --- a/patch-ids.c\n> +++ b/patch-ids.c\n> @@ -7,18 +7,40 @@\n> int commit_patch_id(struct commit *commit, struct diff_options *options,\n> \t\t unsigned char *sha1, int diff_header_only)\n> {\n> -\tif (commit->parents)\n> +\tif (commit->parents) {\n> +\t\tif (commit->parents->next)\n> +\t\t\treturn PATCH_ID_NONE;\n> \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n> \t\t\t commit->object.oid.hash, \"\", options);\n> -\telse\n> +\t} else\n> \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n> \tdiffcore_std(options);\n> -\treturn diff_flush_patch_id(options, sha1, diff_header_only);\n> +\tif (diff_flush_patch_id(options, sha1, diff_header_only))\n> +\t\treturn PATCH_ID_ERROR;\n> +\treturn PATCH_ID_OK;\n> +}\n\nLooks sensible. Thanks.\n"},{"id":"301725","messageId":"20160912155930.2acw4265nfjq3uyj@sigill.intra.peff.net","threadId":"44029","inReplyTo":"xmqqfup8aiud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-12T15:59:31Z","receivedAt":"2016-09-12T15:59:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 02:01:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > And here is v3. Besides commit-message fixups, it drops patch 2, and\n> > instead the third patch teaches commit_patch_id() to distinguish between\n> > errors and \"no patch id\".\n> >\n> > Frankly, I still like v2 better, but I do not feel like arguing with\n> > Johannes about it anymore.\n> \n> FWIW, I too like the simplicity of v2, as all the error-to-die\n> conversion is for cases in which there is no sane recovery path.\n> \n> I'll have to take a bit deeper look at [v3 2/2] that had to become\n> more involved to decide if the additional flexibility is really\n> worth it.\n\nOne other option I didn't really look at: commit_patch_id() could\nconsider feeding it a merge as an error, and it would be come the\ncaller's responsibility to avoid doing so. That should already be the\ncase for \"format-patch --base\".\n\nWe'd probably have to change add_commit_patch_id() and\nhas_commit_patch_id() to return NULL early when fed a merge, but that is\nnot too bad.\n\nThe reason I didn't pursue this is that I didn't want the definition of\n\"what constitutes something with no patch-id\" to cross too many\nabstraction layers. But it's not like we expect a multitude of\nconditions; it will probably remain just \"we don't handle merges\" for\nthe foreseeable future.\n\nThat looks like the patch below (as a replacement for patch 2), which is\neven less invasive. It also performs a little better on my example case,\nbecause we avoid adding merges to the hashmap entirely.\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 77e4663..5d2d96a 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -7,10 +7,12 @@\n int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t unsigned char *sha1, int diff_header_only)\n {\n-\tif (commit->parents)\n+\tif (commit->parents) {\n+\t\tif (commit->parents->next)\n+\t\t\treturn -1;\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n \t\t\t commit->object.oid.hash, \"\", options);\n-\telse\n+\t} else\n \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n \tdiffcore_std(options);\n \treturn diff_flush_patch_id(options, sha1, diff_header_only);\n@@ -72,11 +74,20 @@ static int init_patch_id_entry(struct patch_id *patch,\n \treturn 0;\n }\n \n+static int patch_id_defined(struct commit *commit)\n+{\n+\t/* must be 0 or 1 parents */\n+\treturn !commit->parents || !commit->parents->next;\n+}\n+\n struct patch_id *has_commit_patch_id(struct commit *commit,\n \t\t\t\t struct patch_ids *ids)\n {\n \tstruct patch_id patch;\n \n+\tif (!patch_id_defined(commit))\n+\t\treturn NULL;\n+\n \tmemset(&patch, 0, sizeof(patch));\n \tif (init_patch_id_entry(&patch, commit, ids))\n \t\treturn NULL;\n@@ -89,6 +100,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,\n {\n \tstruct patch_id *key = xcalloc(1, sizeof(*key));\n \n+\tif (!patch_id_defined(commit))\n+\t\treturn NULL;\n+\n \tif (init_patch_id_entry(key, commit, ids)) {\n \t\tfree(key);\n \t\treturn NULL;\n\nI'd probably do a preparatory patch to drop the return value from\nadd_commit_patch_id(). No callers actually look at it.\n\n-Peff\n"},{"id":"301731","messageId":"xmqq37l53ul2.fsf@gitster.mtv.corp.google.com","threadId":"44029","inReplyTo":"20160912155930.2acw4265nfjq3uyj@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T17:18:33Z","receivedAt":"2016-09-12T17:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That looks like the patch below (as a replacement for patch 2), which is\n> even less invasive. It also performs a little better on my example case,\n> because we avoid adding merges to the hashmap entirely.\n\nAfter reading it, I did not find [v3 2/2] is too bad either, but\nthis is even better ;-)\n\n> diff --git a/patch-ids.c b/patch-ids.c\n> index 77e4663..5d2d96a 100644\n> --- a/patch-ids.c\n> +++ b/patch-ids.c\n> @@ -7,10 +7,12 @@\n> int commit_patch_id(struct commit *commit, struct diff_options *options,\n> \t\t unsigned char *sha1, int diff_header_only)\n> {\n> -\tif (commit->parents)\n> +\tif (commit->parents) {\n> +\t\tif (commit->parents->next)\n> +\t\t\treturn -1;\n> \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n> \t\t\t commit->object.oid.hash, \"\", options);\n> -\telse\n> +\t} else\n> \t\tdiff_root_tree_sha1(commit->object.oid.hash, \"\", options);\n\nThis looks familiar ;-)\n\n> @@ -72,11 +74,20 @@ static int init_patch_id_entry(struct patch_id *patch,\n> \treturn 0;\n> }\n> \n> +static int patch_id_defined(struct commit *commit)\n> +{\n> +\t/* must be 0 or 1 parents */\n> +\treturn !commit->parents || !commit->parents->next;\n> +}\n\nIf we make the first hunk begin like so:\n\n> +\tif (commit->parents) {\n> +\t\tif (!patch_id_defined(commit))\n> +\t\t\treturn -1;\n\nI wonder if the compiler gives us the same code.\n\n> struct patch_id *has_commit_patch_id(struct commit *commit,\n> \t\t\t\t struct patch_ids *ids)\n> {\n> \tstruct patch_id patch;\n> \n> +\tif (!patch_id_defined(commit))\n> +\t\treturn NULL;\n> +\n> \tmemset(&patch, 0, sizeof(patch));\n> \tif (init_patch_id_entry(&patch, commit, ids))\n> \t\treturn NULL;\n> @@ -89,6 +100,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,\n> {\n> \tstruct patch_id *key = xcalloc(1, sizeof(*key));\n> \n> +\tif (!patch_id_defined(commit))\n> +\t\treturn NULL;\n> +\n> \tif (init_patch_id_entry(key, commit, ids)) {\n> \t\tfree(key);\n> \t\treturn NULL;\n\nYup, these two hunks look a lot nicer.\n\n> I'd probably do a preparatory patch to drop the return value from\n> add_commit_patch_id(). No callers actually look at it.\n\nThanks.\n"},{"id":"301735","messageId":"20160912175641.nj4h6oupbwaedwbx@sigill.intra.peff.net","threadId":"44029","inReplyTo":"xmqq37l53ul2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-12T17:56:41Z","receivedAt":"2016-09-12T17:56:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2016 at 10:18:33AM -0700, Junio C Hamano wrote:\n\n> > +static int patch_id_defined(struct commit *commit)\n> > +{\n> > +\t/* must be 0 or 1 parents */\n> > +\treturn !commit->parents || !commit->parents->next;\n> > +}\n> \n> If we make the first hunk begin like so:\n> \n> > +\tif (commit->parents) {\n> > +\t\tif (!patch_id_defined(commit))\n> > +\t\t\treturn -1;\n> \n> I wonder if the compiler gives us the same code.\n\nGood idea. I actually put the \"patch_id_defined\" check outside the \"if\"\nblock you've quoted (otherwise we're making assumptions about the\ncontents of patch_id_defined).\n\nI didn't check that the compiler generates the same code, but I'm\nwilling to blindly put faith in it. Either it will inline\npatch_id_defined and optimize out the double-conditional, or it probably\ndoesn't matter in practice. Either way, the compiler is probably smarter\nthan me, and we should shoot for readability and not repeating\nourselves.\n\n> > I'd probably do a preparatory patch to drop the return value from\n> > add_commit_patch_id(). No callers actually look at it.\n\nI decided against this. Technically add_commit_patch_id() can return an\nerror via the header-only diff_flush_patch_id(), and we'd be shutting\nthat down. Of course no callers actually _care_ about that right now, so\nit doesn't matter at this point. But I'd prefer to punt it down the line\nfor when somebody does (and the solution may be to distinguish between\nthose two return codes, or it may be for the caller to have access to\npatch_id_defined(); we won't know until we see the code).\n\nSo here's the replacement for 2/2:\n\n-- >8 --\nSubject: [PATCH] patch-ids: refuse to compute patch-id for merge commit\n\nThe patch-id code which powers \"log --cherry-pick\" doesn't\nlook at whether each commit is a merge or not. It just feeds\nthe commit's first parent to the diff, and ignores any\nadditional parents.\n\nIn theory, this might be useful if you wanted to find\nequivalence between, say, a merge commit and a squash-merge\nthat does the same thing. But it also promotes a false\nequivalence between distinct merges. For example, every\n\"merge -s ours\" would look identical to an empty commit\n(which is true in a sense, but presumably there was a value\nin merging in the discarded history). Since patch-ids are\nmeant for throwing away duplicates, we should err on the\nside of _not_ matching such merges.\n\nMoreover, we may spend a lot of extra time computing these\nmerge diffs. In the case that inspired this patch, a \"git\nformat-patch --cherry-pick\" dropped from over 3 minutes to\nless than 3 seconds.\n\nThis seems pretty drastic, but is easily explained. The\ncommand was invoked by a \"git rebase\" of an older topic\nbranch; there had been tens of thousands of commits on the\nupstream branch in the meantime. In addition, this project\nused a topic-branch workflow with occasional \"back-merges\"\nfrom \"master\" to each topic (to resolve conflicts on the\ntopics rather than in the merge commits). So there were not\nonly extra merges, but the diffs for these back-merges were\ngenerally quite large (because they represented _everything_\nthat had been merged to master since the topic branched).\n\nThis patch treats a merge fed to commit_patch_id() or\nadd_commit_patch_id() as an error, and a lookup for such a\nmerge via has_commit_patch_id() will always return NULL.\nAn earlier version of the patch tried to distinguish between\n\"error\" and \"patch id for merges not defined\", but that\nbecomes unnecessarily complicated. The only callers are:\n\n 1. revision traversals which want to do --cherry-pick;\n they call add_commit_patch_id(), but do not care if it\n fails. They only want to add what we can, look it up\n later with has_commit_patch_id(), and err on the side\n of not-matching.\n\n 2. format-patch --base, which calls commit_patch_id().\n This _does_ notice errors, but should never feed a\n merge in the first place (and if it were to do so\n accidentally, then this patch is a strict improvement;\n we notice the bug rather than generating a bogus\n patch-id).\n\nSo in both cases, this does the right thing.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\n patch-ids.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 77e4663..ce285c2 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -4,9 +4,18 @@\n #include \"sha1-lookup.h\"\n #include \"patch-ids.h\"\n \n+static int patch_id_defined(struct commit *commit)\n+{\n+\t/* must be 0 or 1 parents */\n+\treturn !commit->parents || !commit->parents->next;\n+}\n+\n int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t unsigned char *sha1, int diff_header_only)\n {\n+\tif (!patch_id_defined(commit))\n+\t\treturn -1;\n+\n \tif (commit->parents)\n \t\tdiff_tree_sha1(commit->parents->item->object.oid.hash,\n \t\t\t commit->object.oid.hash, \"\", options);\n@@ -77,6 +86,9 @@ struct patch_id *has_commit_patch_id(struct commit *commit,\n {\n \tstruct patch_id patch;\n \n+\tif (!patch_id_defined(commit))\n+\t\treturn NULL;\n+\n \tmemset(&patch, 0, sizeof(patch));\n \tif (init_patch_id_entry(&patch, commit, ids))\n \t\treturn NULL;\n@@ -89,6 +101,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,\n {\n \tstruct patch_id *key = xcalloc(1, sizeof(*key));\n \n+\tif (!patch_id_defined(commit))\n+\t\treturn NULL;\n+\n \tif (init_patch_id_entry(key, commit, ids)) {\n \t\tfree(key);\n \t\treturn NULL;\n-- \n2.10.0.230.g6f8d04b\n\n"},{"id":"301760","messageId":"xmqqpoo8zw3a.fsf@gitster.mtv.corp.google.com","threadId":"44029","inReplyTo":"20160912175641.nj4h6oupbwaedwbx@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T20:44:57Z","receivedAt":"2016-09-12T20:45:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 12, 2016 at 10:18:33AM -0700, Junio C Hamano wrote:\n>\n>> > +static int patch_id_defined(struct commit *commit)\n>> > +{\n>> > +\t/* must be 0 or 1 parents */\n>> > +\treturn !commit->parents || !commit->parents->next;\n>> > +}\n>> \n>> If we make the first hunk begin like so:\n>> \n>> > +\tif (commit->parents) {\n>> > +\t\tif (!patch_id_defined(commit))\n>> > +\t\t\treturn -1;\n>> \n>> I wonder if the compiler gives us the same code.\n>\n> Good idea. I actually put the \"patch_id_defined\" check outside the \"if\"\n> block you've quoted (otherwise we're making assumptions about the\n> contents of patch_id_defined).\n\nFacepalm. I was mis-reading the condition in the helper function.\nOf course, guarding up-front makes more sense.\n\n"},{"id":"302529","messageId":"alpine.DEB.2.20.1609252023320.129229@virtualbox","threadId":"44029","inReplyTo":"20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] patch-id for merges","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-25T18:25:19Z","receivedAt":"2016-09-25T18:25:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Sep 2016, Jeff King wrote:\n\n> Frankly, I still like v2 better, but I do not feel like arguing with\n> Johannes about it anymore.\n\nSorry that I was a pest. I do care deeply about making our code cleaner,\nthough, which in my mind means less of this \"let exit() clean everything\nup and give the callers no chance to do anything anymore\" approach, which\nto me always felt like we're basically teaching Git to shrug instead of\nbeing useful when something bad happens.\n\nCiao,\nDscho\n"}]}