{"thread":{"id":"45145","subject":"difflame improvements","startedAt":"2017-02-15T05:19:11Z","lastAt":"2017-02-19T06:35:45Z","messageCount":4,"participants":["Edmundo Carmona Antoranz","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"311638","messageId":"CAOc6etYz6+FzDRbkLS2SB9=F9DP18=6uLKdfCN3D3yd2Gug-tw@mail.gmail.com","threadId":"45145","inReplyTo":null,"subject":"difflame improvements","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2017-02-15T05:19:05Z","receivedAt":"2017-02-15T05:19:11Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Hi!\n\nI've been working on detecting revisions where a \"real\" deletion was\nmade and I think I advanced a lot in that front. I still have to work\non many scenarios (renamed files, for example... also performance) but\nat least I'm using a few runs against git-scm history and the results\nare \"promising\":\n\n23:05 $ git blame -s --reverse -L 25,40 HEAD~20..HEAD -- versioncmp.c\n066fb0494e 25) static int initialized;\n066fb0494e 26)\n066fb0494e 27) /*\n8ec68d1ae2 28)  * p1 and p2 point to the first different character in\ntwo strings. If\n8ec68d1ae2 29)  * either p1 or p2 starts with a prerelease suffix, it\nwill be forced\n8ec68d1ae2 30)  * to be on top.\n8ec68d1ae2 31)  *\n8ec68d1ae2 32)  * If both p1 and p2 start with (different) suffix, the order is\n8ec68d1ae2 33)  * determined by config file.\n066fb0494e 34)  *\n8ec68d1ae2 35)  * Note that we don't have to deal with the situation\nwhen both p1 and\n8ec68d1ae2 36)  * p2 start with the same suffix because the common\npart is already\n8ec68d1ae2 37)  * consumed by the caller.\n066fb0494e 38)  *\n066fb0494e 39)  * Return non-zero if *diff contains the return value\nfor versioncmp()\n066fb0494e 40)  */\n\nLines 28-33:\n\n23:05 $ git show --summary 8ec68d1ae2\ncommit 8ec68d1ae2863823b74d67c5e92297e38bbf97bc\nMerge: e801be066 c48886779\nAuthor: Junio C Hamano <>\nDate:   Mon Jan 23 15:59:21 2017 -0800\n\n    Merge branch 'vn/diff-ihc-config'\n\n    \"git diff\" learned diff.interHunkContext configuration variable\n    that gives the default value for its --inter-hunk-context option.\n\n    * vn/diff-ihc-config:\n      diff: add interhunk context config option\n\n\n\nAnd this is not telling me the _real_ revision where the lines were\n_deleted_ so it's not very helpful, as Peff has already mentioned.\n\nRunning difflame:\n\n23:06 $ time ~/proyectos/git/difflame/difflame.py -bp=-s -w HEAD~20\nHEAD -- versioncmp.c\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 80bfd109f..9f81dc106 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -24,42 +24,83 @@\n.\n.\n.\n+b17846432d  33) static void find_better_matching_suffix(const char\n*tagname, const char *suffix,\n+b17846432d  34)                                        int\nsuffix_len, int start, int conf_pos,\n+b17846432d  35)                                        struct\nsuffix_match *match)\n+b17846432d  36) {\nb17846432d  37)        /*\n       c026557a3 versioncmp: generalize version sort suffix reordering\n-c026557a3 (SZEDER 28)  * p1 and p2 point to the first different\ncharacter in two strings. If\n-c026557a3 (SZEDER 29)  * either p1 or p2 starts with a prerelease\nsuffix, it will be forced\n-c026557a3 (SZEDER 30)  * to be on top.\n-c026557a3 (SZEDER 31)  *\n-c026557a3 (SZEDER 32)  * If both p1 and p2 start with (different)\nsuffix, the order is\n-c026557a3 (SZEDER 33)  * determined by config file.\n       b17846432 versioncmp: factor out helper for suffix matching\n+b17846432d  38)         * A better match either starts earlier or\nstarts at the same offset\n+b17846432d  39)         * but is longer.\n+b17846432d  40)         */\n+b17846432d  41)        int end = match->len < suffix_len ?\nmatch->start : match->start-1;\n.\n.\n.\n\nSame range of (deleted) lines:\n\n23:10 $ git --show --name-status c026557a3\ncommit c026557a37361b7019acca28f240a19f546739e9\nAuthor: SZEDER Gábor <>\nDate:   Thu Dec 8 15:24:01 2016 +0100\n\n   versioncmp: generalize version sort suffix reordering\n\n   The 'versionsort.prereleaseSuffix' configuration variable, as its name\n   suggests, is supposed to only deal with tagnames with prerelease\n.\n.\n.\n\n\n   Signed-off-by: SZEDER Gábor <>\n   Signed-off-by: Junio C Hamano <>\n\nM       Documentation/config.txt\nM       Documentation/git-tag.txt\nM       t/t7004-tag.sh\nM       versioncmp.c\n\n\nThis is the revision where the deletion happened.\n\nThat's it for the time being.\n"},{"id":"311841","messageId":"20170217051755.fx2ueizsprw2vida@sigill.intra.peff.net","threadId":"45145","inReplyTo":"CAOc6etYz6+FzDRbkLS2SB9=F9DP18=6uLKdfCN3D3yd2Gug-tw@mail.gmail.com","subject":"Re: difflame improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T05:17:55Z","receivedAt":"2017-02-17T05:18:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 11:19:05PM -0600, Edmundo Carmona Antoranz wrote:\n\n> I've been working on detecting revisions where a \"real\" deletion was\n> made and I think I advanced a lot in that front. I still have to work\n> on many scenarios (renamed files, for example... also performance) but\n> at least I'm using a few runs against git-scm history and the results\n> are \"promising\":\n\nI played with this a bit more, and it did turn up the correct results\nfor some deletions in my experiments.\n\nOne thing I noticed is that it also turned up nonsense for lines that\nblame in weird ways. For instance, I have a diff like this (these are\nreal examples, but don't pay attention to the sha1s; it's in a fork of\ngit, not upstream):\n\n  $ git diff v2.6.5 builtin/prune-packed.c\n  diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n  index 7cf900ea07..5e3727e841 100644\n  --- a/builtin/prune-packed.c\n  +++ b/builtin/prune-packed.c\n  @@ -2,6 +2,7 @@\n   #include \"cache.h\"\n   #include \"progress.h\"\n   #include \"parse-options.h\"\n  +#include \"gh-log.h\"\n   \n   static const char * const prune_packed_usage[] = {\n   \tN_(\"git prune-packed [-n | --dry-run] [-q | --quiet]\"),\n  @@ -29,8 +30,11 @@ static int prune_object(const unsigned char *sha1, const char *path,\n   \n   \tif (*opts & PRUNE_PACKED_DRY_RUN)\n   \t\tprintf(\"rm -f %s\\n\", path);\n  -\telse\n  +\telse {\n  +\t\tgh_logf(\"prune\", \"%s Duplicate loose object pruned\\n\",\n  +\t\t\tsha1_to_hex(sha1));\n   \t\tunlink_or_warn(path);\n  +\t}\n   \treturn 0;\n   }\n   \n\nRunning difflame on it says this:\n\n  $ python /path/to/difflame.py v2.6.5..HEAD -- builtin/prune-packed.c\n  [...]\n  -2c0b29e662 (Jeff King 2016-01-26 15:27:55 -0500 32) \telse\n  +d60032f8640 builtin/prune-packed.c (Jeff King        2015-02-02 23:15:33 -0500 33) \telse {\n  +d60032f8640 builtin/prune-packed.c (Jeff King        2015-02-02 23:15:33 -0500 34) \t\tgh_logf(\"prune\", \"%s Duplicate loose object pruned\\n\",\n  +d60032f8640 builtin/prune-packed.c (Jeff King        2015-02-02 23:15:33 -0500 35) \t\t\tsha1_to_hex(sha1));\n   0d3b729680e builtin/prune-packed.c (Jeff King        2014-10-15 18:40:53 -0400 36) \t\tunlink_or_warn(path);\n  +2396ec85bd1 prune-packed.c         (Linus Torvalds   2005-07-03 14:27:34 -0700 37) \t}\n\nThere are two weird things. One is that the old \"else\" is attributed to\nmy 2c0b29e662. That's quite weird, because that is a merge commit which\ndid not touch the file at all. I haven't tracked it down, but presumably\nthat is weirdness with the --reverse blame.\n\nBut there's another one, that I think is easy to fix. The closing brace\nis attributed to some ancient commit from Linus. Which yes, I'm sure had\na closing brace, but not _my_ closing brace that was added by\nd60032f8640, that the rest of the lines got attributed to.\n\nThis isn't difflame's fault; that's what \"git blame\" tells you about\nthat line. But since I already told difflame \"v2.6.5..HEAD\", it would\nprobably make sense to similarly limit the blame to that range. That\nturns up a boundary commit for the line. Which is _also_ not helpful,\nbut at least the tool is telling me that the line came from before\nv2.6.5, and I don't really need to care much about it.\n\nPart of this is that my use case may be a bit different than yours. I\ndon't actually want to look at the blame results directly. I just want\nto see the set of commits that I'd need to look at and possibly\ncherry-pick in order to re-create the diff.\n\n-Peff\n"},{"id":"311842","messageId":"CAOc6etZxkspqafifjPTbRDoVBt0XuOLbhiuY9bFTD2Wjtxw-HQ@mail.gmail.com","threadId":"45145","inReplyTo":"20170217051755.fx2ueizsprw2vida@sigill.intra.peff.net","subject":"Re: difflame improvements","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2017-02-17T07:01:25Z","receivedAt":"2017-02-17T07:01:35Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Thu, Feb 16, 2017 at 11:17 PM, Jeff King <peff@peff.net> wrote:\n\n> This isn't difflame's fault; that's what \"git blame\" tells you about\n> that line. But since I already told difflame \"v2.6.5..HEAD\", it would\n> probably make sense to similarly limit the blame to that range. That\n> turns up a boundary commit for the line. Which is _also_ not helpful,\n> but at least the tool is telling me that the line came from before\n> v2.6.5, and I don't really need to care much about it.\n\n\nI'm running my own tests on difflame and I have a theory about \"when\"\nit breaks.... at least one of the cases when it breaks:\n\nAnalysis for deleted lines is being driven by git blame --reverse.\nWhat I have noticed is that it \"breaks\" when blame --reverse drives\nthe analysis into revisions where \"treeish1\" is not part of their\nhistory (like, bringing analysis \"to the sides\" of treeish1 instead of\nkeeping analysis in revisions in the history of treeish2 that have\ntreeish1 as one of their ancestors.... which is definitely a valid\ncase for analysis, anyway). In this case, blame --reverse stops being\nhelpful.\n\nTake this example (I just pushed a debug-deletion branch into gh...\nprobably more debugging messages will be needed):\n\n$ difflame.py HEAD~100 HEAD -- Documentation/git-commit.txt\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex f2ab0ee2e..4f8f20a36 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE\" section in linkgit:git-rebase[1].)\nbcf9626a71 (Matthieu Moy      2016-06-28 13:40:11 +0200 265)   If this\noption is specified together with `--amend`, then\n04c8ce9c1c (Markus Heidelberg 2008-12-19 13:14:18 +0100 266)   no\npaths need to be specified, which can be used to amend\nd4ba07cac5 (Johannes Sixt     2008-04-10 13:33:09 +0200 267)   the\nlast commit without committing changes that have\n       Range of revisions: 02db2d..066fb04\n               Treeish1 02db2d04: 02db2d042 Merge branch 'ah/grammos'\n               Treeish2 066fb0494: 066fb0494 blame: draft of line format\n       Blamed Revision afe0e2a39: afe0e2a39 Merge branch\n'da/difftool-dir-diff-fix'\n       Original Filename a/Documentation/git-commit.txt Deleted Line 268\n       Children revisions:\n               3aead1cad7a: 3aead1cad Merge branch 'ak/commit-only-allow-empty'\n       There's only one child revision.... on that revision the line\nwe are tracking is gone\n       Parents of this child revision:\n               afe0e2a39166: afe0e2a39 Merge branch 'da/difftool-dir-diff-fix'\n               beb635ca9ce: beb635ca9 commit: remove 'Clever' message\nfor --only --amend\n       Finding parent where the line has been deleted:\n               beb635ca9: beb635ca9 commit: remove 'Clever' message\nfor --only --amend\n       Range of revisions: 02db2d042..beb635c\n               Treeish1 02db2d0: 02db2d042 Merge branch 'ah/grammos'\n               Treeish2 beb635c: beb635ca9 commit: remove 'Clever'\nmessage for --only --amend\n       Blamed Revision 02db2d0: 02db2d042 Merge branch 'ah/grammos'\n       Original Filename a/Documentation/git-commit.txt Deleted Line 268\n       Children revisions:\n       Found no children... will return the original blamed revision\n(02db2d0) saying that the deleting revision could not be found\n       beb635ca9 commit: remove 'Clever' message for --only --amend\n-beb635ca9 (Andreas Krey 2016-12-09 05:10:21 +0100 268)\nalready been staged.\n       319d83524 commit: make --only --allow-empty work without paths\n+319d835240 (Andreas Krey      2016-12-02 23:15:13 +0100 268)\nalready been staged. ...\n+319d835240 (Andreas Krey      2016-12-02 23:15:13 +0100 269)   paths\nare also not requi...\nd4ba07cac5 (Johannes Sixt     2008-04-10 13:33:09 +0200 270)\n1947bdbc31 (Junio C Hamano    2008-06-22 14:32:27 -0700 271) -u[<mode>]::\n1947bdbc31 (Junio C Hamano    2008-06-22 14:32:27 -0700 272)\n--untracked-files[=<mode>]::\n\n\n\nI know that line 268 was deleted on 319d835240.\n\nSo.... on the first round of merge analysis it says \"let's go into\nbeb635ca9\". That's fine. That's exactly the path that is required to\nreach 319d835240. But then when using this new \"range of revisions\"\nfor git blame --reverse, we get that line 268 is not telling us\nanything useful:\n\n$ git blame --reverse -L268,268 02db2d042..beb635c --\nDocumentation/git-commit.txt\n^02db2d042 (Junio C Hamano 2016-12-19 14:45:30 -0800 268)\nalready been staged.\n\nSo, instead of pointing to 319d835240 (the parent of beb635c), it's\nbasically saying something like \"I give up\". My hunch (haven't sat\ndown to digest all the details about the output of git blame\n--reverse... YET) is that, given that 02db2d042 is _not_ part of the\nhistory of beb635c, git blame reverse is trying to tell me just\nthat... and that means I'll have to \"script around this scenario\".\n\n$ git merge-base 02db2d042 beb635c\n0202c411edc25940cc381bf317badcdf67670be4\n\n\nThanks in advance.\n"},{"id":"312070","messageId":"CAOc6etY39Pi72U3ibtF94oh4xqzP4SB3VEj8QJOhX-GobTOTRA@mail.gmail.com","threadId":"45145","inReplyTo":"CAOc6etZxkspqafifjPTbRDoVBt0XuOLbhiuY9bFTD2Wjtxw-HQ@mail.gmail.com","subject":"Re: difflame improvements","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2017-02-19T06:35:39Z","receivedAt":"2017-02-19T06:35:45Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Fri, Feb 17, 2017 at 1:01 AM, Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> On Thu, Feb 16, 2017 at 11:17 PM, Jeff King <peff@peff.net> wrote:\n>\n>> This isn't difflame's fault; that's what \"git blame\" tells you about\n>> that line. But since I already told difflame \"v2.6.5..HEAD\", it would\n>> probably make sense to similarly limit the blame to that range. That\n>> turns up a boundary commit for the line. Which is _also_ not helpful,\n>> but at least the tool is telling me that the line came from before\n>> v2.6.5, and I don't really need to care much about it.\n>\n>\n> I'm running my own tests on difflame and I have a theory about \"when\"\n> it breaks.... at least one of the cases when it breaks:\n>\n> Analysis for deleted lines is being driven by git blame --reverse.\n> What I have noticed is that it \"breaks\" when blame --reverse drives\n> the analysis into revisions where \"treeish1\" is not part of their\n> history (like, bringing analysis \"to the sides\" of treeish1 instead of\n> keeping analysis in revisions in the history of treeish2 that have\n> treeish1 as one of their ancestors.... which is definitely a valid\n> case for analysis, anyway). In this case, blame --reverse stops being\n> helpful.\n>\n\nAt the cost of being slower, I just pushed to master the best results yet.\n\nThe workaround I developed for the case I described on the previous\nmail ended up providing much better results overall so I ended up\nreplacing the whole merge-analysis logic with it.\n\nThanks for your kind help and comments, Peff. Let me know how it goes.\n"}]}