{"thread":{"id":"40811","subject":"[PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","startedAt":"2015-11-17T01:29:22Z","lastAt":"2015-11-20T04:38:18Z","messageCount":9,"participants":["Edmundo Carmona Antoranz","Eric Sunshine","Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"273405","messageId":"1447723762-32309-1-git-send-email-eantoranz@gmail.com","threadId":"40811","inReplyTo":null,"subject":"[PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-17T01:29:22Z","receivedAt":"2015-11-17T01:29:22Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"If a file has been deleted/renamed, blame refused to display\nblame content even if the path provided does match an existing\nblob on said revision.\n\n$ git status\nOn branch hide\nChanges not staged for commit:\n (use \"git add/rm ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\n deleted: testfile1.txt\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nBefore:\n$ git blame testfile1.txt\nfatal: cannot stat path 'testfile1.txt': No such file or directory\n$ git blame testfile1.txt HEAD\nfatal: cannot stat path 'testfile1.txt': No such file or directory\n\nAfter:\n$ git blame testfile1.txt\nfatal: Cannot lstat 'testfile1.txt': No such file or directory\n$ git blame testfile1.txt HEAD\n^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 1) testfile 2\n^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 2)\n^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 3) Some content\n\nSigned-off-by: Edmundo Carmona Antoranz \n---\n builtin/blame.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 83612f5..856971a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2683,12 +2683,13 @@ parse_done:\n \t\targv[argc - 1] = \"--\";\n \n \t\tsetup_work_tree();\n-\t\tif (!file_exists(path))\n-\t\t\tdie_errno(\"cannot stat path '%s'\", path);\n \t}\n \n \trevs.disable_stdin = 1;\n \tsetup_revisions(argc, argv, &revs, NULL);\n+\tif (!revs.pending.nr && !file_exists(path))\n+\t\tdie_errno(\"cannot stat path '%s'\", path);\n+\n \tmemset(&sb, 0, sizeof(sb));\n \n \tsb.revs = &revs;\n-- \n2.6.2\n"},{"id":"273406","messageId":"CAPig+cTjg8yvmLO0uuWZt4nATUnfxJOj4f_AmvRP9mHsGRmEOg@mail.gmail.com","threadId":"40811","inReplyTo":"1447723762-32309-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-17T05:11:25Z","receivedAt":"2015-11-17T05:11:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 16, 2015 at 8:29 PM, Edmundo Carmona Antoranz\n wrote:\n> blame: avoid checking if a file exists on the working tree\n> if a revision is provided\n\nThis subject is a bit long; try to keep it to about 72 characters or less.\n\nMore importantly, though, it doesn't give us a high level overview of\nthe purpose of the patch, which is that it is fixing blame to work on\na file at a given revision even if the file no longer exists in the\nworktree.\n\n> If a file has been deleted/renamed, blame refused to display\n\nImperative: s/refused/refuses/\n\n> blame content even if the path provided does match an existing\n> blob on said revision.\n\ngit-blame documentation does not advertise \"blame \" as a\nvalid invocation. It does advertise \"blame -- \", and this\ncase already works correctly even when does not exist in the\nworktree.\n\ngit-annotate documentation, on the other hand, does advertise\n\"annotate \", and it fails to work when is absent\nfrom the worktree, so perhaps you want to sell this patch as fixing\ngit-annotate instead?\n\n> $ git status\n> On branch hide\n> Changes not staged for commit:\n> (use \"git add/rm ...\" to update what will be committed)\n> (use \"git checkout -- ...\" to discard changes in working directory)\n>\n> deleted: testfile1.txt\n>\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>\n> Before:\n> $ git blame testfile1.txt\n> fatal: cannot stat path 'testfile1.txt': No such file or directory\n> $ git blame testfile1.txt HEAD\n> fatal: cannot stat path 'testfile1.txt': No such file or directory\n>\n> After:\n> $ git blame testfile1.txt\n> fatal: Cannot lstat 'testfile1.txt': No such file or directory\n> $ git blame testfile1.txt HEAD\n> ^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 1) testfile 2\n> ^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 2)\n> ^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 3) Some content\n\nThis example is certainly illustrative, but an even more common case\nmay be trying to blame/annotate a file which existed in an older\nrevision but doesn't exist anymore at HEAD, thus is absent from the\nworktree. Such a case could likely be described in a sentence or two\nwithout resorting to verbose examples (though, not a big deal if you\nkeep the example).\n\nA new test or two would be welcome, possibly in t/annotate-tests.sh if\nyou consider this also fixing git-blame, or in t8001-annotate.sh if\nyou're selling it only as a fix for git-annotate.\n\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -2683,12 +2683,13 @@ parse_done:\n> argv[argc - 1] = \"--\";\n>\n> setup_work_tree();\n> - if (!file_exists(path))\n> - die_errno(\"cannot stat path '%s'\", path);\n> }\n>\n> revs.disable_stdin = 1;\n> setup_revisions(argc, argv, &revs, NULL);\n> + if (!revs.pending.nr && !file_exists(path))\n> + die_errno(\"cannot stat path '%s'\", path);\n> +\n> memset(&sb, 0, sizeof(sb));\n>\n> sb.revs = &revs;\n> --\n> 2.6.2\n"},{"id":"273450","messageId":"20151117224809.GE27862@sigill.intra.peff.net","threadId":"40811","inReplyTo":"CAPig+cTjg8yvmLO0uuWZt4nATUnfxJOj4f_AmvRP9mHsGRmEOg@mail.gmail.com","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-17T22:48:10Z","receivedAt":"2015-11-17T22:48:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:\n\n> > blame content even if the path provided does match an existing\n> > blob on said revision.\n> \n> git-blame documentation does not advertise \"blame \" as a\n> valid invocation. It does advertise \"blame -- \", and this\n> case already works correctly even when does not exist in the\n> worktree.\n\nHmm. Out of curiosity I tried:\n\n git blame v2.4.0 -- t/t6031-merge-recursive.sh\n\nand it segfaults. This bisects to Max's recent 1b0d400 (blame: extract\nfind_single_final, 2015-10-30), but I do not see anything obviously\nwrong with it from a quick glance.\n\n-Peff\n"},{"id":"273452","messageId":"CAGZ79kbM7ZezVFSt7oHYppd=CcgoQMCyYqXB6H1k-rK82kzUPg@mail.gmail.com","threadId":"40811","inReplyTo":"20151117224809.GE27862@sigill.intra.peff.net","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-17T23:00:08Z","receivedAt":"2015-11-17T23:00:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 17, 2015 at 2:48 PM, Jeff King wrote:\n> On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:\n>\n>> > blame content even if the path provided does match an existing\n>> > blob on said revision.\n>>\n>> git-blame documentation does not advertise \"blame \" as a\n>> valid invocation. It does advertise \"blame -- \", and this\n>> case already works correctly even when does not exist in the\n>> worktree.\n>\n> Hmm. Out of curiosity I tried:\n>\n> git blame v2.4.0 -- t/t6031-merge-recursive.sh\n>\n> and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract\n> find_single_final, 2015-10-30), but I do not see anything obviously\n> wrong with it from a quick glance.\n\nit did not fail when running through gdb, so I conclude it is a memory issue\n(like using an uninitialized pointer, or memory allocation too small).\n\nvalgrind produces:\n\n==18444== Process terminating with default action of signal 11 (SIGSEGV)\n==18444== General Protection Fault\n==18444== at 0x4032121: strcmp (valgrind/memcheck/mc_replace_strmem.c:725)\n==18444== by 0x41A86F: get_origin\n(/usr/local/google/home/sbeller/OSS/git/builtin/blame.c:483)\n==18444== by 0x4201EF: cmd_blame\n(/usr/local/google/home/sbeller/OSS/git/builtin/blame.c:2763)\n==18444== by 0x405896: run_builtin\n(/usr/local/google/home/sbeller/OSS/git/git.c:350)\n==18444== by 0x405AA4: handle_builtin\n(/usr/local/google/home/sbeller/OSS/git/git.c:536)\n==18444== by 0x405BC0: run_argv\n(/usr/local/google/home/sbeller/OSS/git/git.c:582)\n==18444== by 0x405DB8: main\n(/usr/local/google/home/sbeller/OSS/git/git.c:690)\n==18444==\n\n\n>\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"273453","messageId":"CAPig+cQcmtZJmm_FGMNL4OCTgdFZGyLjVrwbfCCp+0kwBeFuRQ@mail.gmail.com","threadId":"40811","inReplyTo":"20151117224809.GE27862@sigill.intra.peff.net","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-17T23:01:25Z","receivedAt":"2015-11-17T23:01:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 17, 2015 at 5:48 PM, Jeff King wrote:\n> On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:\n>> > blame content even if the path provided does match an existing\n>> > blob on said revision.\n>>\n>> git-blame documentation does not advertise \"blame \" as a\n>> valid invocation. It does advertise \"blame -- \", and this\n>> case already works correctly even when does not exist in the\n>> worktree.\n>\n> Hmm. Out of curiosity I tried:\n>\n> git blame v2.4.0 -- t/t6031-merge-recursive.sh\n>\n> and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract\n> find_single_final, 2015-10-30), but I do not see anything obviously\n> wrong with it from a quick glance.\n\nIn the original code, sb->final received was assigned value of obj,\nwhich may have gone through deref_tag(), however, after 1b0d400,\nsb->final is unconditionally assigned the original value of obj, not\nthe (potentially) deref'd value.\n"},{"id":"273454","messageId":"20151117232237.GA29014@sigill.intra.peff.net","threadId":"40811","inReplyTo":"CAPig+cQcmtZJmm_FGMNL4OCTgdFZGyLjVrwbfCCp+0kwBeFuRQ@mail.gmail.com","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-17T23:22:37Z","receivedAt":"2015-11-17T23:22:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:\n\n> On Tue, Nov 17, 2015 at 5:48 PM, Jeff King wrote:\n> > On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:\n> >> > blame content even if the path provided does match an existing\n> >> > blob on said revision.\n> >>\n> >> git-blame documentation does not advertise \"blame \" as a\n> >> valid invocation. It does advertise \"blame -- \", and this\n> >> case already works correctly even when does not exist in the\n> >> worktree.\n> >\n> > Hmm. Out of curiosity I tried:\n> >\n> > git blame v2.4.0 -- t/t6031-merge-recursive.sh\n> >\n> > and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract\n> > find_single_final, 2015-10-30), but I do not see anything obviously\n> > wrong with it from a quick glance.\n> \n> In the original code, sb->final received was assigned value of obj,\n> which may have gone through deref_tag(), however, after 1b0d400,\n> sb->final is unconditionally assigned the original value of obj, not\n> the (potentially) deref'd value.\n\nGood eye. I fed it a tag, find_single_final knows that points to a\ncommit, then prepare_final casts the tag object to a commit. Whoops.\n\nThe patch below fixes it for me. It probably needs a test, but I have to\nrun for the moment.\n\n-- >8 --\nSubject: [PATCH] blame: fix object casting regression\n\nCommit 1b0d400 refactored the prepare_final() function so\nthat it could be reused in multiple places. Originally, the\nloop had two outputs: a commit to stuff into sb->final, and\nthe name of the commit from the rev->pending array.\n\nAfter the refactor, that loop is put in its own function\nwith a single return value: the object_array_entry from the\nrev->pending array. This contains both the name and the object,\nbut with one important difference: the object is the\n_original_ object found by the revision parser, not the\ndereferenced commit. If one feeds a tag to \"git blame\", we\nend up casting the tag object to a \"struct commit\", which\ncauses a segfault.\n\nInstead, let's return the commit (properly casted) directly\nfrom the function, and take the \"name\" as an optional\nout-parameter. This does the right thing, and actually\nsimplifies the callers, who no longer need to cast or\ndereference the object_array_entry themselves.\n\nSigned-off-by: Jeff King \n---\n builtin/blame.c | 30 ++++++++++++++----------------\n 1 file changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex ac36738..2184e39 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2403,10 +2403,12 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \treturn commit;\n }\n \n-static struct object_array_entry *find_single_final(struct rev_info *revs)\n+static struct commit *find_single_final(struct rev_info *revs,\n+\t\t\t\t\tconst char **name_p)\n {\n \tint i;\n-\tstruct object_array_entry *found = NULL;\n+\tstruct commit *found = NULL;\n+\tconst char *name = NULL;\n \n \tfor (i = 0; i < revs->pending.nr; i++) {\n \t\tstruct object *obj = revs->pending.objects[i].item;\n@@ -2418,22 +2420,20 @@ static struct object_array_entry *find_single_final(struct rev_info *revs)\n \t\t\tdie(\"Non commit %s?\", revs->pending.objects[i].name);\n \t\tif (found)\n \t\t\tdie(\"More than one commit to dig from %s and %s?\",\n-\t\t\t revs->pending.objects[i].name,\n-\t\t\t found->name);\n-\t\tfound = &(revs->pending.objects[i]);\n+\t\t\t revs->pending.objects[i].name, name);\n+\t\tfound = (struct commit *)obj;\n+\t\tname = revs->pending.objects[i].name;\n \t}\n+\tif (name_p)\n+\t\t*name_p = name;\n \treturn found;\n }\n \n static char *prepare_final(struct scoreboard *sb)\n {\n-\tstruct object_array_entry *found = find_single_final(sb->revs);\n-\tif (found) {\n-\t\tsb->final = (struct commit *) found->item;\n-\t\treturn xstrdup(found->name);\n-\t} else {\n-\t\treturn NULL;\n-\t}\n+\tconst char *name;\n+\tsb->final = find_single_final(sb->revs, &name);\n+\treturn xstrdup_or_null(name);\n }\n \n static char *prepare_initial(struct scoreboard *sb)\n@@ -2721,11 +2721,9 @@ parse_done:\n \t\tdie(\"Cannot use --contents with final commit object name\");\n \n \tif (reverse && revs.first_parent_only) {\n-\t\tstruct object_array_entry *entry = find_single_final(sb.revs);\n-\t\tif (!entry)\n+\t\tfinal_commit = find_single_final(sb.revs, NULL);\n+\t\tif (!final_commit)\n \t\t\tdie(\"--reverse and --first-parent together require specified latest commit\");\n-\t\telse\n-\t\t\tfinal_commit = (struct commit*) entry->item;\n \t}\n \n \t/*\n-- \n2.6.3.636.g1460207\n"},{"id":"273455","messageId":"CAOc6etbjCYRp4gbSNRtcngwbh93R67JYZHJn0=D=FeiBm5=FNw@mail.gmail.com","threadId":"40811","inReplyTo":"CAPig+cTjg8yvmLO0uuWZt4nATUnfxJOj4f_AmvRP9mHsGRmEOg@mail.gmail.com","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-17T23:37:45Z","receivedAt":"2015-11-17T23:37:45Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Mon, Nov 16, 2015 at 11:11 PM, Eric Sunshine wrote:\n>\n> This subject is a bit long; try to keep it to about 72 characters or less.\n>\n> More importantly, though, it doesn't give us a high level overview of\n> the purpose of the patch, which is that it is fixing blame to work on\n> a file at a given revision even if the file no longer exists in the\n> worktree.\n\nSure!\n\n> git-blame documentation does not advertise \"blame \" as a\n> valid invocation. It does advertise \"blame -- \", and this\n> case already works correctly even when does not exist in the\n> worktree.\n>\n> git-annotate documentation, on the other hand, does advertise\n> \"annotate \", and it fails to work when is absent\n> from the worktree, so perhaps you want to sell this patch as fixing\n> git-annotate instead?\n\nSo, do I forget about the blame patch (given that I'm not fixing an\nadvertised syntax, even if it's supported) and fix annotate instead or\ndo I fix both? And if I should swing for both, do I create a single\npatch or a chain of two patches, one for each builtin?\n\n> This example is certainly illustrative, but an even more common case\n> may be trying to blame/annotate a file which existed in an older\n> revision but doesn't exist anymore at HEAD, thus is absent from the\n> worktree. Such a case could likely be described in a sentence or two\n> without resorting to verbose examples (though, not a big deal if you\n> keep the example).\n\nK.\n\n>\n> A new test or two would be welcome, possibly in t/annotate-tests.sh if\n> you consider this also fixing git-blame, or in t8001-annotate.sh if\n> you're selling it only as a fix for git-annotate.\n\nI guess I'll wait for feedback about my first question before I decide\nwhat I will do about the tests.\n\nThank you very much for your comments, Eric.\n"},{"id":"273457","messageId":"CAOc6etY9cXm04KdqUdRE11xtG+7Yj3ihkB4-OTweyWyGF8gs0g@mail.gmail.com","threadId":"40811","inReplyTo":"CAOc6etbjCYRp4gbSNRtcngwbh93R67JYZHJn0=D=FeiBm5=FNw@mail.gmail.com","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-17T23:47:01Z","receivedAt":"2015-11-17T23:47:01Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Tue, Nov 17, 2015 at 5:37 PM, Edmundo Carmona Antoranz\n wrote:\n> So, do I forget about the blame patch (given that I'm not fixing an\n> advertised syntax, even if it's supported) and fix annotate instead or\n> do I fix both? And if I should swing for both, do I create a single\n> patch or a chain of two patches, one for each builtin?\n\nActually, cmd_annotate will call cmd_blame so this patch actually\nfixes annotate as well (nice unintended consequence).\n\nSo I guess it will be a single patch. Let me work on the tests and\nthen I'll send a patch that will hopefully cover all raised points.\n\nCheers!\n"},{"id":"273522","messageId":"CAPig+cTaOTfwzodKSabdy9HFbF66RuEXwmvjZ8QuQVFMaVpA7w@mail.gmail.com","threadId":"40811","inReplyTo":"20151117232237.GA29014@sigill.intra.peff.net","subject":"Re: [PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-20T04:38:18Z","receivedAt":"2015-11-20T04:38:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 17, 2015 at 6:22 PM, Jeff King wrote:\n> On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:\n>> On Tue, Nov 17, 2015 at 5:48 PM, Jeff King wrote:\n>> > Hmm. Out of curiosity I tried:\n>> >\n>> > git blame v2.4.0 -- t/t6031-merge-recursive.sh\n>> >\n>> > and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract\n>> > find_single_final, 2015-10-30), but I do not see anything obviously\n>> > wrong with it from a quick glance.\n>>\n>> In the original code, sb->final received was assigned value of obj,\n>> which may have gone through deref_tag(), however, after 1b0d400,\n>> sb->final is unconditionally assigned the original value of obj, not\n>> the (potentially) deref'd value.\n>\n> Good eye. I fed it a tag, find_single_final knows that points to a\n> commit, then prepare_final casts the tag object to a commit. Whoops.\n>\n> The patch below fixes it for me. It probably needs a test, but I have to\n> run for the moment.\n\nSorry for the late response. This patch mirrors my thoughts on fixing\nthe bug, and appears correct. For what it's worth:\n\nReviewed-by: Eric Sunshine \n\n> -- >8 --\n> Subject: [PATCH] blame: fix object casting regression\n>\n> Commit 1b0d400 refactored the prepare_final() function so\n> that it could be reused in multiple places. Originally, the\n> loop had two outputs: a commit to stuff into sb->final, and\n> the name of the commit from the rev->pending array.\n>\n> After the refactor, that loop is put in its own function\n> with a single return value: the object_array_entry from the\n> rev->pending array. This contains both the name and the object,\n> but with one important difference: the object is the\n> _original_ object found by the revision parser, not the\n> dereferenced commit. If one feeds a tag to \"git blame\", we\n> end up casting the tag object to a \"struct commit\", which\n> causes a segfault.\n>\n> Instead, let's return the commit (properly casted) directly\n> from the function, and take the \"name\" as an optional\n> out-parameter. This does the right thing, and actually\n> simplifies the callers, who no longer need to cast or\n> dereference the object_array_entry themselves.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/blame.c | 30 ++++++++++++++----------------\n> 1 file changed, 14 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index ac36738..2184e39 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2403,10 +2403,12 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> return commit;\n> }\n>\n> -static struct object_array_entry *find_single_final(struct rev_info *revs)\n> +static struct commit *find_single_final(struct rev_info *revs,\n> + const char **name_p)\n> {\n> int i;\n> - struct object_array_entry *found = NULL;\n> + struct commit *found = NULL;\n> + const char *name = NULL;\n>\n> for (i = 0; i < revs->pending.nr; i++) {\n> struct object *obj = revs->pending.objects[i].item;\n> @@ -2418,22 +2420,20 @@ static struct object_array_entry *find_single_final(struct rev_info *revs)\n> die(\"Non commit %s?\", revs->pending.objects[i].name);\n> if (found)\n> die(\"More than one commit to dig from %s and %s?\",\n> - revs->pending.objects[i].name,\n> - found->name);\n> - found = &(revs->pending.objects[i]);\n> + revs->pending.objects[i].name, name);\n> + found = (struct commit *)obj;\n> + name = revs->pending.objects[i].name;\n> }\n> + if (name_p)\n> + *name_p = name;\n> return found;\n> }\n>\n> static char *prepare_final(struct scoreboard *sb)\n> {\n> - struct object_array_entry *found = find_single_final(sb->revs);\n> - if (found) {\n> - sb->final = (struct commit *) found->item;\n> - return xstrdup(found->name);\n> - } else {\n> - return NULL;\n> - }\n> + const char *name;\n> + sb->final = find_single_final(sb->revs, &name);\n> + return xstrdup_or_null(name);\n> }\n>\n> static char *prepare_initial(struct scoreboard *sb)\n> @@ -2721,11 +2721,9 @@ parse_done:\n> die(\"Cannot use --contents with final commit object name\");\n>\n> if (reverse && revs.first_parent_only) {\n> - struct object_array_entry *entry = find_single_final(sb.revs);\n> - if (!entry)\n> + final_commit = find_single_final(sb.revs, NULL);\n> + if (!final_commit)\n> die(\"--reverse and --first-parent together require specified latest commit\");\n> - else\n> - final_commit = (struct commit*) entry->item;\n> }\n>\n> /*\n> --\n> 2.6.3.636.g1460207\n>\n"}]}