{"thread":{"id":"23760","subject":"[tig PATCH] fix off-by-one on parent selection","startedAt":"2010-05-10T08:55:04Z","lastAt":"2010-06-10T02:23:56Z","messageCount":9,"participants":["Jeff King","Jonas Fonseca"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"141360","messageId":"20100510085504.GA2283@coredump.intra.peff.net","threadId":"23760","inReplyTo":null,"subject":"[tig PATCH] fix off-by-one on parent selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-10T08:55:04Z","receivedAt":"2010-05-10T08:55:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally, we use \"git rev-list -1 --parents\" to get the\nlist of parents, and therefore the 0th slot was the commit\nin question, the 1st slot was the 1st parent, and so forth.\n\nCommit 0a46941 switched this to use --pretty=format:%P, so\nthat the menu-selection code could be easily used (which\ncounts items starting from 0). However, we only use the menu\ncode in the case of multiple parents.  For a single parent,\nthis introduced an off-by-one where we look just past the\nparent we want.\n\nThis patch fixes it by explicitly selecting the 0th parent\nfor the single parent case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is an old bug, but I finally got a chance to track it down.\n\nThere is a related buglet elsewhere in select_commit_parent. Now that we\nask git to print only the parents, we will get no output at all for a\nparent-less commit. This will cause iobuf_read to return an error, and\nwe will print \"Failed to get parent information\" instead of \"The\nselected commit has no parents\" (or \"Path '%s' does not exist\" if we are\nblaming the parent of a commit that introduced a file).\n\nAFAICT, fixing it would mean improving iobuf_read to differentiate \"no\noutput\" from \"there were errors\". I'll leave that sort of infrastructure\nrefactoring to you if you want to do it. The resulting bug is quite\nminor.\n\n tig.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/tig.c b/tig.c\nindex 074e414..60932d4 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -4012,7 +4012,9 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n \t\treturn FALSE;\n \t}\n \n-\tif (parents > 1 && !open_commit_parent_menu(buf, &parents))\n+\tif (parents == 1)\n+\t\tparents = 0;\n+\telse if (!open_commit_parent_menu(buf, &parents))\n \t\treturn FALSE;\n \n \tstring_copy_rev(rev, &buf[41 * parents]);\n-- \n1.7.1.248.gf52fc\n"},{"id":"142109","messageId":"AANLkTim8cQ-1oBE-BOwbjTlyn2E2V64NvM_6Drs3kTAS@mail.gmail.com","threadId":"23760","inReplyTo":"20100510085504.GA2283@coredump.intra.peff.net","subject":"Re: [tig PATCH] fix off-by-one on parent selection","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2010-05-22T17:19:12Z","receivedAt":"2010-05-22T17:19:12Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"On Mon, May 10, 2010 at 04:55, Jeff King <peff@peff.net> wrote:\n> This patch fixes it by explicitly selecting the 0th parent\n> for the single parent case.\n> [...]\n> This is an old bug, but I finally got a chance to track it down.\n\nThanks for fixing it.\n\n> There is a related buglet elsewhere in select_commit_parent. Now that we\n> ask git to print only the parents, we will get no output at all for a\n> parent-less commit. This will cause iobuf_read to return an error, and\n> we will print \"Failed to get parent information\" instead of \"The\n> selected commit has no parents\" (or \"Path '%s' does not exist\" if we are\n> blaming the parent of a commit that introduced a file).\n>\n> AFAICT, fixing it would mean improving iobuf_read to differentiate \"no\n> output\" from \"there were errors\". I'll leave that sort of infrastructure\n> refactoring to you if you want to do it. The resulting bug is quite\n> minor.\n\nThe spaced damaged patch below fixes the first error.\n--- >8 --- >8 --- >8 ---\ndiff --git a/tig.c b/tig.c\nindex 35b0cfa..f5bb1b9 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -1028,7 +1028,7 @@ io_read_buf(struct io *io, char buf[], size_t bufsize)\n                string_ncopy_do(buf, bufsize, result, strlen(result));\n        }\n\n-       return io_done(io) && result;\n+       return io_done(io) && !io_error(io);\n }\n\n static bool\n--- 8< --- 8< --- 8< ---\n\nHowever, it seems that the output of the command that was previously\nused for fetching parents and the current one pretty printing using\nthe %P flag is also the cause of the breakage.\n\nIn the tig repository, trying to \"blame\" the parent of b801d8b2b shows\nreproduces the problem. Commit b801d8b2b replaced cgit.c with tig.c,\nwhich means there is no parent blame to show.\n\nBefore:\n> git rev-list -1 --parents b801d8b2b -- tig.c\nb801d8b2bc1a6aac6b9744f21f7a10a51e16c53e\n.. i.e no parents as expected.\n\nNow:\n> git log --no-color -1 --pretty=format:%P b801d8b2b -- tig.c\na7bc4b1447f974fbbe400c3657d9ec3d0fda133e\n.. i.e. the parent of b801d8b2b, but where tig.c does not exist.\n\nThe attached patch addresses this problem by reverting back to the\ncommand used before.\n\nA related question is why the hell I chose to switch to using %P in\ncommit 0a4694191613f887151a52f0c70e6b6181ea5fb6 ...\n\n--\nJonas Fonseca\n\n\n tig.c |   12 ++++--------\n 1 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/tig.c b/tig.c\nindex 35b0cfa..b2dbca7 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -3995,17 +3995,15 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n {\n \tchar buf[SIZEOF_STR * 4];\n \tconst char *revlist_argv[] = {\n-\t\t\"git\", \"log\", \"--no-color\", \"-1\",\n-\t\t\t\"--pretty=format:%P\", id, \"--\", path, NULL\n+\t\t\"git\", \"rev-list\", \"-1\", \"--parents\", id, \"--\", path, NULL\n \t};\n \tint parents;\n \n-\tif (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||\n-\t    (parents = strlen(buf) / 40) < 0) {\n+\tif (!io_run_buf(revlist_argv, buf, sizeof(buf))) {\n \t\treport(\"Failed to get parent information\");\n \t\treturn FALSE;\n \n-\t} else if (parents == 0) {\n+\t} else if ((parents = (strlen(buf) / 40) - 1) <= 0) {\n \t\tif (path)\n \t\t\treport(\"Path '%s' does not exist in the parent\", path);\n \t\telse\n@@ -4013,9 +4011,7 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n \t\treturn FALSE;\n \t}\n \n-\tif (parents == 1)\n-\t\tparents = 0;\n-\telse if (!open_commit_parent_menu(buf, &parents))\n+\tif (parents > 1 && !open_commit_parent_menu(buf, &parents))\n \t\treturn FALSE;\n \n \tstring_copy_rev(rev, &buf[41 * parents]);\n"},{"id":"142129","messageId":"20100523074051.GA16730@coredump.intra.peff.net","threadId":"23760","inReplyTo":"AANLkTim8cQ-1oBE-BOwbjTlyn2E2V64NvM_6Drs3kTAS@mail.gmail.com","subject":"Re: [tig PATCH] fix off-by-one on parent selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-23T07:40:52Z","receivedAt":"2010-05-23T07:40:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 22, 2010 at 01:19:12PM -0400, Jonas Fonseca wrote:\n\n> > AFAICT, fixing it would mean improving iobuf_read to differentiate \"no\n> > output\" from \"there were errors\". I'll leave that sort of infrastructure\n> > refactoring to you if you want to do it. The resulting bug is quite\n> > minor.\n> \n> The spaced damaged patch below fixes the first error.\n> --- >8 --- >8 --- >8 ---\n> diff --git a/tig.c b/tig.c\n> index 35b0cfa..f5bb1b9 100644\n> --- a/tig.c\n> +++ b/tig.c\n> @@ -1028,7 +1028,7 @@ io_read_buf(struct io *io, char buf[], size_t bufsize)\n>                 string_ncopy_do(buf, bufsize, result, strlen(result));\n>         }\n> \n> -       return io_done(io) && result;\n> +       return io_done(io) && !io_error(io);\n>  }\n> \n>  static bool\n> --- 8< --- 8< --- 8< ---\n\nYeah, that works for me. I was hesitant to do it because I wasn't sure\nif other callers were relying on that behavior, but it looks like all\ncalls properly check the output for sanity.\n\n> However, it seems that the output of the command that was previously\n> used for fetching parents and the current one pretty printing using\n> the %P flag is also the cause of the breakage.\n> \n> In the tig repository, trying to \"blame\" the parent of b801d8b2b shows\n> reproduces the problem. Commit b801d8b2b replaced cgit.c with tig.c,\n> which means there is no parent blame to show.\n> \n> Before:\n> > git rev-list -1 --parents b801d8b2b -- tig.c\n> b801d8b2bc1a6aac6b9744f21f7a10a51e16c53e\n> .. i.e no parents as expected.\n> \n> Now:\n> > git log --no-color -1 --pretty=format:%P b801d8b2b -- tig.c\n> a7bc4b1447f974fbbe400c3657d9ec3d0fda133e\n> .. i.e. the parent of b801d8b2b, but where tig.c does not exist.\n\nThis confused me at first, because those outputs should be the same, but\nnow I see: the pretty %P does not respect history simplification, so you\nget the _true_ parent of b801d8b2b, and not the simplified one when\nlimiting history to 'tig.c'.\n\nSo yes, the rev-list version is better, because it at least realizes\nthat tig.c has no parent.\n\nBut I think neither is ideal. What we probably want is to detect the\nrename between the two, and do the blame from the parent using cgit.c.\ngit-blame will already recognize content coming from another file and\ngive us that filename, but we are moving to the parent ourselves, so we\nhave to do that rename detection manually (IOW, this behavior triggers\n_only_ when you are trying to blame the parent of a commit which\nsimultaneously introduced the line and moved the filename).\n\nPatch is below. After writing it, I realized that tig.c is not\nactually a rename of cgit.c (they are too dissimilar). It does provide\nthe correct \"Path 'tig.c' does not exist in the parent\". And you can see\nthe rename-following behavior with something like:\n\n  perl -e 'print \"$_\\n\" for 1..1000' >old\n  git add . && git commit -m added\n  perl -pe 's/^1$/foo/' <old >new\n  git add new && git rm old && git commit -m moved\n\nNow try \"tig blame new\". For all of the lines but the first, blaming the\nparent gets you the correct \"The selected commit has no parents\". But\nparent-blaming the first line will correctly re-blame using the filename\n\"old\".\n\nThere are some possible optimizations that I didn't implement:\n\n  1. With this patch, we always check for a rename to the parent. But we\n     really only need to do so if the commit in question introduced the\n     file. One way to detect that is by first running \"git diff-tree\n     $file\", and only doing the rename detection if the file was added.\n     We could also potentially use the rev-list in select_commit_parent\n     to see that we have no parents. That would mean combining\n     select_commit_parent and my new follow_parent_rename.\n\n  2. The diff-tree could potentially be combined with the one we execute\n     immediately after in setup_blame_parent_line. But I don't think it\n     is worth it. In the rename-follow, we have to look at _all_ of the\n     files, as they are potential sources. But in\n     setup_blame_parent_line, we are generating diffs and can restrict\n     our diff to only the file of interest. I don't think there is a way\n     to say \"consider all files as rename sources, but only show the\n     patch for this one file\".\n\ndiff --git a/tig.c b/tig.c\nindex 28679f9..cfa26ce 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -3991,12 +3991,12 @@ open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)\n }\n \n static bool\n-select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n+select_commit_parent(const char *id, char rev[SIZEOF_REV])\n {\n \tchar buf[SIZEOF_STR * 4];\n \tconst char *revlist_argv[] = {\n \t\t\"git\", \"log\", \"--no-color\", \"-1\",\n-\t\t\t\"--pretty=format:%P\", id, \"--\", path, NULL\n+\t\t\t\"--pretty=format:%P\", id, \"--\", NULL\n \t};\n \tint parents;\n \n@@ -4006,10 +4006,7 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n \t\treturn FALSE;\n \n \t} else if (parents == 0) {\n-\t\tif (path)\n-\t\t\treport(\"Path '%s' does not exist in the parent\", path);\n-\t\telse\n-\t\t\treport(\"The selected commit has no parents\");\n+\t\treport(\"The selected commit has no parents\");\n \t\treturn FALSE;\n \t}\n \n@@ -4022,6 +4019,48 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n \treturn TRUE;\n }\n \n+static int\n+follow_parent_rename(const char *id, const char *parent, const char *dest,\n+\t\t     char source[SIZEOF_STR])\n+{\n+\tconst char *diff_argv[] = {\n+\t\t\"git\", \"diff-tree\", \"-z\", \"--name-status\", \"-M\",\n+\t\tparent, id, \"--\", NULL\n+\t};\n+\tstruct io io = {};\n+\tchar *buf;\n+\n+\tif (!io_run(&io, diff_argv, opt_cdup, IO_RD))\n+\t\treturn FALSE;\n+\n+\twhile ((buf = io_get(&io, 0, TRUE))) {\n+\t\tint status = buf[0];\n+\n+\t\tif (!(buf = io_get(&io, 0, TRUE)))\n+\t\t\tbreak;\n+\n+\t\tif (status == 'A' && !strcmp(buf, dest)) {\n+\t\t\treport(\"Path '%s' does not exist in the parent\", dest);\n+\t\t\tio_done(&io);\n+\t\t\treturn FALSE;\n+\t\t}\n+\n+\t\tif (status == 'R') {\n+\t\t\tstring_ncopy_do(source, SIZEOF_STR, buf, strlen(buf));\n+\t\t\tif (!(buf = io_get(&io, 0, TRUE)))\n+\t\t\t\tbreak;\n+\t\t\tif (!strcmp(buf, dest)) {\n+\t\t\t\tio_done(&io);\n+\t\t\t\treturn TRUE;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tstring_ncopy_do(source, SIZEOF_STR, dest, strlen(dest));\n+\tio_done(&io);\n+\treturn TRUE;\n+}\n+\n /*\n  * Pager backend\n  */\n@@ -5190,9 +5229,9 @@ blame_request(struct view *view, enum request request, struct line *line)\n \n \tcase REQ_PARENT:\n \t\tif (check_blame_commit(blame, TRUE) &&\n-\t\t    select_commit_parent(blame->commit->id, opt_ref,\n-\t\t\t\t\t blame->commit->filename)) {\n-\t\t\tstring_copy(opt_file, blame->commit->filename);\n+\t\t    select_commit_parent(blame->commit->id, opt_ref) &&\n+\t\t    follow_parent_rename(blame->commit->id, opt_ref,\n+\t\t\t\t\t blame->commit->filename, opt_file)) {\n \t\t\tsetup_blame_parent_line(view, blame);\n \t\t\topen_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);\n \t\t}\n"},{"id":"142130","messageId":"20100523075503.GA24598@coredump.intra.peff.net","threadId":"23760","inReplyTo":"20100523074051.GA16730@coredump.intra.peff.net","subject":"Re: [tig PATCH] fix off-by-one on parent selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-23T07:55:03Z","receivedAt":"2010-05-23T07:55:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 23, 2010 at 03:40:52AM -0400, Jeff King wrote:\n\n> Now try \"tig blame new\". For all of the lines but the first, blaming the\n> parent gets you the correct \"The selected commit has no parents\". But\n> parent-blaming the first line will correctly re-blame using the filename\n> \"old\".\n\nBy the way, there is one minor bug remaining after this patch:\n\n>  \tcase REQ_PARENT:\n>  \t\tif (check_blame_commit(blame, TRUE) &&\n> -\t\t    select_commit_parent(blame->commit->id, opt_ref,\n> -\t\t\t\t\t blame->commit->filename)) {\n> -\t\t\tstring_copy(opt_file, blame->commit->filename);\n> +\t\t    select_commit_parent(blame->commit->id, opt_ref) &&\n> +\t\t    follow_parent_rename(blame->commit->id, opt_ref,\n> +\t\t\t\t\t blame->commit->filename, opt_file)) {\n>  \t\t\tsetup_blame_parent_line(view, blame);\n>  \t\t\topen_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);\n>  \t\t}\n\nWe may write some new filename into opt_file in the follow_parent_rename\ncall, but setup_blame_parent_line always diffs the original file. Which\nmeans we lose the line position when following a rename.\n\nWe need to do the equivalent of:\n\n  git diff -U0 \\\n    opt_ref:opt_file \\\n    blame->commit->id:blame->commit->filename\n\nIOW, to blame directly between the two blobs. Sadly, I don't think there\nis a plumbing command to do this, so we are stuck using regular \"git\ndiff\", which may have surprises in the config.\n\nThe patch below works for my simple tests.  I think we probably want to\nbe doing this anyway for the multiple-parent case. I didn't test, but I\ndon't think that diff-tree invocation is going to produce any output for\na merge commit.\n\ndiff --git a/tig.c b/tig.c\nindex cfa26ce..4388c2f 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -5177,15 +5177,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)\n static void\n setup_blame_parent_line(struct view *view, struct blame *blame)\n {\n+\tchar from[SIZEOF_REF+SIZEOF_STR];\n+\tchar to[SIZEOF_REF+SIZEOF_STR];\n \tconst char *diff_tree_argv[] = {\n-\t\t\"git\", \"diff-tree\", \"-U0\", blame->commit->id,\n-\t\t\t\"--\", blame->commit->filename, NULL\n+\t\t\"git\", \"diff\", \"--no-textconv\", \"--no-extdiff\", \"--no-color\",\n+\t\t\"-U0\", from, to, \"--\", NULL\n \t};\n \tstruct io io = {};\n \tint parent_lineno = -1;\n \tint blamed_lineno = -1;\n \tchar *line;\n \n+\tsnprintf(from, sizeof(from), \"%s:%s\", opt_ref, opt_file);\n+\tsnprintf(to, sizeof(to), \"%s:%s\", blame->commit->id,\n+\t\t blame->commit->filename);\n+\n \tif (!io_run(&io, diff_tree_argv, NULL, IO_RD))\n \t\treturn;\n \n"},{"id":"143055","messageId":"1275767765-8509-1-git-send-email-fonseca@diku.dk","threadId":"23760","inReplyTo":"20100523075503.GA24598@coredump.intra.peff.net","subject":"[PATCH] Improve parent blame to detect renames by using the previous information","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2010-06-05T19:56:05Z","receivedAt":"2010-06-05T19:56:05Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":">From git commit 96e117099c0e4f7d508eb071f60b6275038f6f37:\n\n It gives the parent commit of the blamed commit, _and_ a path in that\n parent commit that corresponds to the blamed path --- in short, it is\n the origin that would have been blamed (or passed blame through) for\n the line _if_ the blamed commit did not change that line.\n\nThis functionality was released in git version 1.6.3 in 2009-05-06.\n---\n NEWS  |    2 +\n tig.c |   99 +++++++++++++++-------------------------------------------------\n 2 files changed, 25 insertions(+), 76 deletions(-)\n\n I finally got some more time to dig around this. What if we simply uses\n the information given by the porcelain output's previous line? It\n handles your simple test case, and navigating in the tig repository. It\n also makes it possible to delete a lot of code.\n\ndiff --git a/NEWS b/NEWS\nindex 190e5dc..499bdbc 100644\n--- a/NEWS\n+++ b/NEWS\n@@ -38,6 +38,8 @@ Bug fixes:\n  - Fix unbind to behave as if the keybinding was never defined.\n  - Fix unbind to also cover built-in run requests.\n  - Fix parsing of unknown keymap names.\n+ - Blame view: fix parent blame to detect renames. It uses \"previous\"\n+   line info from the blame porcelain output added in git version 1.6.3.\n \n tig-0.15\n --------\ndiff --git a/tig.c b/tig.c\nindex 01f48c3..044da28 100644\n--- a/tig.c\n+++ b/tig.c\n@@ -3977,73 +3977,6 @@ parse_author_line(char *ident, const char **author, struct time *time)\n \t}\n }\n \n-static bool\n-open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)\n-{\n-\tchar rev[SIZEOF_REV];\n-\tconst char *revlist_argv[] = {\n-\t\t\"git\", \"log\", \"--no-color\", \"-1\", \"--pretty=format:%s\", rev, NULL\n-\t};\n-\tstruct menu_item *items;\n-\tchar text[SIZEOF_STR];\n-\tbool ok = TRUE;\n-\tint i;\n-\n-\titems = calloc(*parents + 1, sizeof(*items));\n-\tif (!items)\n-\t\treturn FALSE;\n-\n-\tfor (i = 0; i < *parents; i++) {\n-\t\tstring_copy_rev(rev, &buf[SIZEOF_REV * i]);\n-\t\tif (!io_run_buf(revlist_argv, text, sizeof(text)) ||\n-\t\t    !(items[i].text = strdup(text))) {\n-\t\t\tok = FALSE;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tif (ok) {\n-\t\t*parents = 0;\n-\t\tok = prompt_menu(\"Select parent\", items, parents);\n-\t}\n-\tfor (i = 0; items[i].text; i++)\n-\t\tfree((char *) items[i].text);\n-\tfree(items);\n-\treturn ok;\n-}\n-\n-static bool\n-select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n-{\n-\tchar buf[SIZEOF_STR * 4];\n-\tconst char *revlist_argv[] = {\n-\t\t\"git\", \"log\", \"--no-color\", \"-1\",\n-\t\t\t\"--pretty=format:%P\", id, \"--\", path, NULL\n-\t};\n-\tint parents;\n-\n-\tif (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||\n-\t    (parents = strlen(buf) / 40) < 0) {\n-\t\treport(\"Failed to get parent information\");\n-\t\treturn FALSE;\n-\n-\t} else if (parents == 0) {\n-\t\tif (path)\n-\t\t\treport(\"Path '%s' does not exist in the parent\", path);\n-\t\telse\n-\t\t\treport(\"The selected commit has no parents\");\n-\t\treturn FALSE;\n-\t}\n-\n-\tif (parents == 1)\n-\t\tparents = 0;\n-\telse if (!open_commit_parent_menu(buf, &parents))\n-\t\treturn FALSE;\n-\n-\tstring_copy_rev(rev, &buf[41 * parents]);\n-\treturn TRUE;\n-}\n-\n /*\n  * Pager backend\n  */\n@@ -4898,7 +4831,8 @@ struct blame_commit {\n \tconst char *author;\t\t/* Author of the commit. */\n \tstruct time time;\t\t/* Date from the author ident. */\n \tchar filename[128];\t\t/* Name of file. */\n-\tbool has_previous;\t\t/* Was a \"previous\" line detected. */\n+\tchar parent_id[SIZEOF_REV];\t/* Parent/previous SHA1 ID. */\n+\tchar parent_filename[128];\t/* Parent/previous name of file. */\n };\n \n struct blame {\n@@ -5097,7 +5031,11 @@ blame_read(struct view *view, char *line)\n \t\tstring_ncopy(commit->title, line, strlen(line));\n \n \t} else if (match_blame_header(\"previous \", &line)) {\n-\t\tcommit->has_previous = TRUE;\n+\t\tif (strlen(line) <= SIZEOF_REV)\n+\t\t\treturn FALSE;\n+\t\tstring_copy_rev(commit->parent_id, line);\n+\t\tline += SIZEOF_REV;\n+\t\tstring_ncopy(commit->parent_filename, line, strlen(line));\n \n \t} else if (match_blame_header(\"filename \", &line)) {\n \t\tstring_ncopy(commit->filename, line, strlen(line));\n@@ -5153,15 +5091,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)\n static void\n setup_blame_parent_line(struct view *view, struct blame *blame)\n {\n+\tchar from[SIZEOF_REF + SIZEOF_STR];\n+\tchar to[SIZEOF_REF + SIZEOF_STR];\n \tconst char *diff_tree_argv[] = {\n-\t\t\"git\", \"diff-tree\", \"-U0\", blame->commit->id,\n-\t\t\t\"--\", blame->commit->filename, NULL\n+\t\t\"git\", \"diff\", \"--no-textconv\", \"--no-extdiff\", \"--no-color\",\n+\t\t\t\"-U0\", from, to, \"--\", NULL\n \t};\n \tstruct io io;\n \tint parent_lineno = -1;\n \tint blamed_lineno = -1;\n \tchar *line;\n \n+\tsnprintf(from, sizeof(from), \"%s:%s\", opt_ref, opt_file);\n+\tsnprintf(to, sizeof(to), \"%s:%s\", blame->commit->id,\n+\t\t blame->commit->filename);\n+\n \tif (!io_run(&io, IO_RD, NULL, diff_tree_argv))\n \t\treturn;\n \n@@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)\n \t\tbreak;\n \n \tcase REQ_PARENT:\n-\t\tif (check_blame_commit(blame, TRUE) &&\n-\t\t    select_commit_parent(blame->commit->id, opt_ref,\n-\t\t\t\t\t blame->commit->filename)) {\n-\t\t\tstring_copy(opt_file, blame->commit->filename);\n+\t\tif (!check_blame_commit(blame, TRUE))\n+\t\t\tbreak;\n+\t\tif (!*blame->commit->parent_id) {\n+\t\t\treport(\"The selected commit has no parents\");\n+\t\t} else {\n+\t\t\tstring_copy_rev(opt_ref, blame->commit->parent_id);\n+\t\t\tstring_copy_rev(opt_file, blame->commit->parent_filename);\n \t\t\tsetup_blame_parent_line(view, blame);\n \t\t\topen_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);\n \t\t}\n@@ -5228,7 +5175,7 @@ blame_request(struct view *view, enum request request, struct line *line)\n \t\t\t\t\t\"-C\", \"-M\", \"HEAD\", \"--\", view->vid, NULL\n \t\t\t};\n \n-\t\t\tif (!blame->commit->has_previous) {\n+\t\t\tif (!*blame->commit->parent_id) {\n \t\t\t\tdiff_index_argv[1] = \"diff\";\n \t\t\t\tdiff_index_argv[2] = \"--no-color\";\n \t\t\t\tdiff_index_argv[6] = \"--\";\n-- \n1.7.1.354.ge64bd\n"},{"id":"143056","messageId":"AANLkTilNKxzRhLoYzPopjFYfubJpWw7Gu4Jc-RGLosa1@mail.gmail.com","threadId":"23760","inReplyTo":"1275767765-8509-1-git-send-email-fonseca@diku.dk","subject":"Re: [TIG PATCH] Improve parent blame to detect renames by using the previous information","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2010-06-05T19:57:05Z","receivedAt":"2010-06-05T19:57:05Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Sorry, forgot to prefix this mail properly ...\n\nOn Sat, Jun 5, 2010 at 15:56, Jonas Fonseca <fonseca@diku.dk> wrote:\n> From git commit 96e117099c0e4f7d508eb071f60b6275038f6f37:\n>\n>  It gives the parent commit of the blamed commit, _and_ a path in that\n>  parent commit that corresponds to the blamed path --- in short, it is\n>  the origin that would have been blamed (or passed blame through) for\n>  the line _if_ the blamed commit did not change that line.\n>\n> This functionality was released in git version 1.6.3 in 2009-05-06.\n> ---\n>  NEWS  |    2 +\n>  tig.c |   99 +++++++++++++++-------------------------------------------------\n>  2 files changed, 25 insertions(+), 76 deletions(-)\n>\n>  I finally got some more time to dig around this. What if we simply uses\n>  the information given by the porcelain output's previous line? It\n>  handles your simple test case, and navigating in the tig repository. It\n>  also makes it possible to delete a lot of code.\n>\n> diff --git a/NEWS b/NEWS\n> index 190e5dc..499bdbc 100644\n> --- a/NEWS\n> +++ b/NEWS\n> @@ -38,6 +38,8 @@ Bug fixes:\n>  - Fix unbind to behave as if the keybinding was never defined.\n>  - Fix unbind to also cover built-in run requests.\n>  - Fix parsing of unknown keymap names.\n> + - Blame view: fix parent blame to detect renames. It uses \"previous\"\n> +   line info from the blame porcelain output added in git version 1.6.3.\n>\n>  tig-0.15\n>  --------\n> diff --git a/tig.c b/tig.c\n> index 01f48c3..044da28 100644\n> --- a/tig.c\n> +++ b/tig.c\n> @@ -3977,73 +3977,6 @@ parse_author_line(char *ident, const char **author, struct time *time)\n>        }\n>  }\n>\n> -static bool\n> -open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)\n> -{\n> -       char rev[SIZEOF_REV];\n> -       const char *revlist_argv[] = {\n> -               \"git\", \"log\", \"--no-color\", \"-1\", \"--pretty=format:%s\", rev, NULL\n> -       };\n> -       struct menu_item *items;\n> -       char text[SIZEOF_STR];\n> -       bool ok = TRUE;\n> -       int i;\n> -\n> -       items = calloc(*parents + 1, sizeof(*items));\n> -       if (!items)\n> -               return FALSE;\n> -\n> -       for (i = 0; i < *parents; i++) {\n> -               string_copy_rev(rev, &buf[SIZEOF_REV * i]);\n> -               if (!io_run_buf(revlist_argv, text, sizeof(text)) ||\n> -                   !(items[i].text = strdup(text))) {\n> -                       ok = FALSE;\n> -                       break;\n> -               }\n> -       }\n> -\n> -       if (ok) {\n> -               *parents = 0;\n> -               ok = prompt_menu(\"Select parent\", items, parents);\n> -       }\n> -       for (i = 0; items[i].text; i++)\n> -               free((char *) items[i].text);\n> -       free(items);\n> -       return ok;\n> -}\n> -\n> -static bool\n> -select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)\n> -{\n> -       char buf[SIZEOF_STR * 4];\n> -       const char *revlist_argv[] = {\n> -               \"git\", \"log\", \"--no-color\", \"-1\",\n> -                       \"--pretty=format:%P\", id, \"--\", path, NULL\n> -       };\n> -       int parents;\n> -\n> -       if (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||\n> -           (parents = strlen(buf) / 40) < 0) {\n> -               report(\"Failed to get parent information\");\n> -               return FALSE;\n> -\n> -       } else if (parents == 0) {\n> -               if (path)\n> -                       report(\"Path '%s' does not exist in the parent\", path);\n> -               else\n> -                       report(\"The selected commit has no parents\");\n> -               return FALSE;\n> -       }\n> -\n> -       if (parents == 1)\n> -               parents = 0;\n> -       else if (!open_commit_parent_menu(buf, &parents))\n> -               return FALSE;\n> -\n> -       string_copy_rev(rev, &buf[41 * parents]);\n> -       return TRUE;\n> -}\n> -\n>  /*\n>  * Pager backend\n>  */\n> @@ -4898,7 +4831,8 @@ struct blame_commit {\n>        const char *author;             /* Author of the commit. */\n>        struct time time;               /* Date from the author ident. */\n>        char filename[128];             /* Name of file. */\n> -       bool has_previous;              /* Was a \"previous\" line detected. */\n> +       char parent_id[SIZEOF_REV];     /* Parent/previous SHA1 ID. */\n> +       char parent_filename[128];      /* Parent/previous name of file. */\n>  };\n>\n>  struct blame {\n> @@ -5097,7 +5031,11 @@ blame_read(struct view *view, char *line)\n>                string_ncopy(commit->title, line, strlen(line));\n>\n>        } else if (match_blame_header(\"previous \", &line)) {\n> -               commit->has_previous = TRUE;\n> +               if (strlen(line) <= SIZEOF_REV)\n> +                       return FALSE;\n> +               string_copy_rev(commit->parent_id, line);\n> +               line += SIZEOF_REV;\n> +               string_ncopy(commit->parent_filename, line, strlen(line));\n>\n>        } else if (match_blame_header(\"filename \", &line)) {\n>                string_ncopy(commit->filename, line, strlen(line));\n> @@ -5153,15 +5091,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)\n>  static void\n>  setup_blame_parent_line(struct view *view, struct blame *blame)\n>  {\n> +       char from[SIZEOF_REF + SIZEOF_STR];\n> +       char to[SIZEOF_REF + SIZEOF_STR];\n>        const char *diff_tree_argv[] = {\n> -               \"git\", \"diff-tree\", \"-U0\", blame->commit->id,\n> -                       \"--\", blame->commit->filename, NULL\n> +               \"git\", \"diff\", \"--no-textconv\", \"--no-extdiff\", \"--no-color\",\n> +                       \"-U0\", from, to, \"--\", NULL\n>        };\n>        struct io io;\n>        int parent_lineno = -1;\n>        int blamed_lineno = -1;\n>        char *line;\n>\n> +       snprintf(from, sizeof(from), \"%s:%s\", opt_ref, opt_file);\n> +       snprintf(to, sizeof(to), \"%s:%s\", blame->commit->id,\n> +                blame->commit->filename);\n> +\n>        if (!io_run(&io, IO_RD, NULL, diff_tree_argv))\n>                return;\n>\n> @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)\n>                break;\n>\n>        case REQ_PARENT:\n> -               if (check_blame_commit(blame, TRUE) &&\n> -                   select_commit_parent(blame->commit->id, opt_ref,\n> -                                        blame->commit->filename)) {\n> -                       string_copy(opt_file, blame->commit->filename);\n> +               if (!check_blame_commit(blame, TRUE))\n> +                       break;\n> +               if (!*blame->commit->parent_id) {\n> +                       report(\"The selected commit has no parents\");\n> +               } else {\n> +                       string_copy_rev(opt_ref, blame->commit->parent_id);\n> +                       string_copy_rev(opt_file, blame->commit->parent_filename);\n>                        setup_blame_parent_line(view, blame);\n>                        open_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);\n>                }\n> @@ -5228,7 +5175,7 @@ blame_request(struct view *view, enum request request, struct line *line)\n>                                        \"-C\", \"-M\", \"HEAD\", \"--\", view->vid, NULL\n>                        };\n>\n> -                       if (!blame->commit->has_previous) {\n> +                       if (!*blame->commit->parent_id) {\n>                                diff_index_argv[1] = \"diff\";\n>                                diff_index_argv[2] = \"--no-color\";\n>                                diff_index_argv[6] = \"--\";\n> --\n> 1.7.1.354.ge64bd\n>\n>\n\n\n\n-- \nJonas Fonseca\n"},{"id":"143124","messageId":"20100606223545.GA11424@coredump.intra.peff.net","threadId":"23760","inReplyTo":"1275767765-8509-1-git-send-email-fonseca@diku.dk","subject":"Re: [PATCH] Improve parent blame to detect renames by using the previous information","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-06T22:35:45Z","receivedAt":"2010-06-06T22:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:\n\n>  I finally got some more time to dig around this. What if we simply uses\n>  the information given by the porcelain output's previous line? It\n>  handles your simple test case, and navigating in the tig repository. It\n>  also makes it possible to delete a lot of code.\n\nYes, I think that is the right way to go. The whole time I was doing the\nother patches, I kept thinking that we had something like this in the\nblame output, but when I looked I couldn't find it (which I can't see\nhow I would manage now, it's quite obvious to see).\n\nSo I think it does the right thing, and I see you also included my fix:\n\n> +\tchar from[SIZEOF_REF + SIZEOF_STR];\n> +\tchar to[SIZEOF_REF + SIZEOF_STR];\n>  \tconst char *diff_tree_argv[] = {\n> -\t\t\"git\", \"diff-tree\", \"-U0\", blame->commit->id,\n> -\t\t\t\"--\", blame->commit->filename, NULL\n> +\t\t\"git\", \"diff\", \"--no-textconv\", \"--no-extdiff\", \"--no-color\",\n> +\t\t\t\"-U0\", from, to, \"--\", NULL\n>  \t};\n>  \tstruct io io;\n>  \tint parent_lineno = -1;\n>  \tint blamed_lineno = -1;\n>  \tchar *line;\n>  \n> +\tsnprintf(from, sizeof(from), \"%s:%s\", opt_ref, opt_file);\n> +\tsnprintf(to, sizeof(to), \"%s:%s\", blame->commit->id,\n> +\t\t blame->commit->filename);\n> +\n\nto handle the line-jumping properly.\n\nOne minor bug:\n\n> @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)\n>  \t\tbreak;\n>  \n>  \tcase REQ_PARENT:\n> -\t\tif (check_blame_commit(blame, TRUE) &&\n> -\t\t    select_commit_parent(blame->commit->id, opt_ref,\n> -\t\t\t\t\t blame->commit->filename)) {\n> -\t\t\tstring_copy(opt_file, blame->commit->filename);\n> +\t\tif (!check_blame_commit(blame, TRUE))\n> +\t\t\tbreak;\n> +\t\tif (!*blame->commit->parent_id) {\n> +\t\t\treport(\"The selected commit has no parents\");\n> +\t\t} else {\n> +\t\t\tstring_copy_rev(opt_ref, blame->commit->parent_id);\n> +\t\t\tstring_copy_rev(opt_file, blame->commit->parent_filename);\n\nThis second string_copy_rev should be a string_ncopy, shouldn't it?\n\n-Peff\n"},{"id":"143352","messageId":"AANLkTimGXAXg7fi3zKD3f-pIht0q0qYPp_6ivlt8LgDF@mail.gmail.com","threadId":"23760","inReplyTo":"20100606223545.GA11424@coredump.intra.peff.net","subject":"Re: [PATCH] Improve parent blame to detect renames by using the previous information","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2010-06-09T16:10:12Z","receivedAt":"2010-06-09T16:10:12Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"On Sun, Jun 6, 2010 at 18:35, Jeff King <peff@peff.net> wrote:\n>\n> On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:\n>\n> One minor bug:\n>\n> > @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)\n> >               break;\n> >\n> >       case REQ_PARENT:\n> > -             if (check_blame_commit(blame, TRUE) &&\n> > -                 select_commit_parent(blame->commit->id, opt_ref,\n> > -                                      blame->commit->filename)) {\n> > -                     string_copy(opt_file, blame->commit->filename);\n> > +             if (!check_blame_commit(blame, TRUE))\n> > +                     break;\n> > +             if (!*blame->commit->parent_id) {\n> > +                     report(\"The selected commit has no parents\");\n> > +             } else {\n> > +                     string_copy_rev(opt_ref, blame->commit->parent_id);\n> > +                     string_copy_rev(opt_file, blame->commit->parent_filename);\n>\n> This second string_copy_rev should be a string_ncopy, shouldn't it?\n\nOh, yes, a regular copy and paste bug. Thanks for noticing. I will\ninclude this and consider tagging another release.\n\n--\nJonas Fonseca\n"},{"id":"143390","messageId":"AANLkTilpMcHi4ZrrEmG4hi3TVo1IK4C-skq8cRADvAPf@mail.gmail.com","threadId":"23760","inReplyTo":"20100606223545.GA11424@coredump.intra.peff.net","subject":"Re: [PATCH] Improve parent blame to detect renames by using the previous information","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2010-06-10T02:23:56Z","receivedAt":"2010-06-10T02:23:56Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"On Sun, Jun 6, 2010 at 18:35, Jeff King <peff@peff.net> wrote:\n> On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:\n>\n> So I think it does the right thing, and I see you also included my fix:\n>\n>> +     char from[SIZEOF_REF + SIZEOF_STR];\n>> +     char to[SIZEOF_REF + SIZEOF_STR];\n>>       const char *diff_tree_argv[] = {\n>> -             \"git\", \"diff-tree\", \"-U0\", blame->commit->id,\n>> -                     \"--\", blame->commit->filename, NULL\n>> +             \"git\", \"diff\", \"--no-textconv\", \"--no-extdiff\", \"--no-color\",\n>> +                     \"-U0\", from, to, \"--\", NULL\n>>       };\n>>       struct io io;\n>>       int parent_lineno = -1;\n>>       int blamed_lineno = -1;\n>>       char *line;\n>>\n>> +     snprintf(from, sizeof(from), \"%s:%s\", opt_ref, opt_file);\n>> +     snprintf(to, sizeof(to), \"%s:%s\", blame->commit->id,\n>> +              blame->commit->filename);\n>> +\n>\n> to handle the line-jumping properly.\n\nYes, I accidently squashed everything together in the patch I sent.\nBefore pushing the final version I move this part to a separate commit\nwith you as the author and made it use string_format.\n\n-- \nJonas Fonseca\n"}]}