{"thread":{"id":"44820","subject":"[PATCH 0/3] fixing some random blame corner cases","startedAt":"2017-01-06T04:17:15Z","lastAt":"2017-03-27T22:11:23Z","messageCount":8,"participants":["Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"308838","messageId":"20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net","threadId":"44820","inReplyTo":null,"subject":"[PATCH 0/3] fixing some random blame corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-06T04:15:41Z","receivedAt":"2017-01-06T04:17:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are three fixes for some fairly obscure corner cases. I haven't\nactually seen these in the wild. I came up with the final one while\ndiscussing a hypothetical with somebody, then ran across the middle one\nwhile trying to write a test for the third, which made me scratch my\nhead enough to yield the first one. Classic yak-shaving.\n\nOne other thing that surprised me while writing blame tests is that\n\"--root\" is not the default for git-blame (though it has been for many\nyears in git-log). I'm not sure if it would be a good idea to change it,\nor if blame is too plumbing-ish to allow that.\n\n  [1/3]: blame: fix alignment with --abbrev=40\n  [2/3]: blame: handle --no-abbrev\n  [3/3]: blame: output porcelain \"previous\" header for each file\n\n builtin/blame.c             |  27 ++++++----\n t/t8002-blame.sh            |  32 ++++++++++++\n t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 166 insertions(+), 10 deletions(-)\n create mode 100755 t/t8011-blame-split-file.sh\n\n-Peff\n"},{"id":"308839","messageId":"20170106041808.bd6ts7tkhjqyju7r@sigill.intra.peff.net","threadId":"44820","inReplyTo":"20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net","subject":"[PATCH 2/3] blame: handle --no-abbrev","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-06T04:18:08Z","receivedAt":"2017-01-06T04:18:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can already ask blame for full sha1s with \"-l\" or with\n\"--abbrev=40\". But for consistency with other parts of Git,\nwe should support \"--no-abbrev\".\n\nWorse, blame already accepts --no-abbrev, but it's totally\nbroken. When we see --no-abbrev, the abbrev variable is set\nto 0, which is then used as a printf precision. For regular\nsha1s, that means we print nothing at all (which is very\nwrong). For boundary commits we decrement it to \"-1\", which\nprintf interprets as \"no limit\" (which is almost correct,\nexcept it misses the 39-length magic explained in the\nprevious commit).\n\nLet's detect --no-abbrev and behave as if --abbrev=40 was\ngiven.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also wondered if we needed to clamp this within MINIMUM_ABBREV, but\nthat is done for us already by the parseopt handler.\n\n builtin/blame.c  | 2 ++\n t/t8002-blame.sh | 4 ++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1d312542de..c6170fed81 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2659,6 +2659,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tif (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n+\telse if (!abbrev)\n+\t\tabbrev = GIT_SHA1_HEXSZ;\n \n \tif (revs_file && read_ancestry(revs_file))\n \t\tdie_errno(\"reading graft file '%s' failed\", revs_file);\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex c6347ad8fd..380e1c1054 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -114,4 +114,8 @@ test_expect_success 'blame --abbrev=40 behaves like -l' '\n \tcheck_abbrev 39 --abbrev=40 ^HEAD\n '\n \n+test_expect_success '--no-abbrev works like --abbrev=40' '\n+\tcheck_abbrev 40 --no-abbrev\n+'\n+\n test_done\n-- \n2.11.0.519.g31435224cf\n\n"},{"id":"308840","messageId":"20170106041740.cgyful3263fpnvi2@sigill.intra.peff.net","threadId":"44820","inReplyTo":"20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net","subject":"[PATCH 1/3] blame: fix alignment with --abbrev=40","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-06T04:17:40Z","receivedAt":"2017-01-06T04:18:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The blame command internally adds 1 to any requested sha1\nabbreviation length, and then subtracts it when outputting a\nboundary commit. This lets regular and boundary sha1s line\nup visually, but it misses one corner case.\n\nWhen the requested length is 40, we bump the value to 41.\nBut since we only have 40 characters, that's all we can show\n(fortunately the truncation is done by a printf precision\nfield, so it never tries to read past the end of the\nbuffer).  So a normal sha1 shows 40 hex characters, and a\nboundary sha1 shows \"^\" plus 40 hex characters. The result\nis misaligned.\n\nThe \"-l\" option to show long sha1s gets around this by\nskipping the \"abbrev\" variable entirely and just always\nusing GIT_SHA1_HEXSZ.  This avoids the \"+1\" issue, but it\ndoes mean that boundary commits only have 39 characters\nprinted.  This is somewhat odd, but it does look good\nvisually: the results are aligned and left-justified. The\nalternative would be to allocate an extra column that would\ncontain either an extra space or the \"^\" boundary marker.\n\nAs this is by definition the human-readable view, it's\nprobably not that big a deal either way (and of course\n--porcelain, etc, correctly produce correct 40-hex sha1s).\nBut for consistency, this patch teaches --abbrev=40 to\nproduce the same output as \"-l\" (always left-aligned, with\n40-hex for normal sha1s, and \"^\" plus 39-hex for\nboundaries).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI mostly didn't explore the extra-column solution out of a sense of\ninertia. The \"-l\" option has behaved this way for years. But it was also\nout of laziness, as I doubt anybody cares too much, and it would require\na fair bit of special-casing in the printing routines.\n\n builtin/blame.c  |  2 +-\n t/t8002-blame.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4ddfadb71f..1d312542de 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2656,7 +2656,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n-\tif (0 < abbrev)\n+\tif (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n \ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex ab79de9544..c6347ad8fd 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -86,4 +86,32 @@ test_expect_success 'blame with showEmail config true' '\n \ttest_cmp expected_n result\n '\n \n+test_expect_success 'set up abbrev tests' '\n+\ttest_commit abbrev &&\n+\tsha1=$(git rev-parse --verify HEAD) &&\n+\tcheck_abbrev () {\n+\t\texpect=$1; shift\n+\t\techo $sha1 | cut -c 1-$expect >expect &&\n+\t\tgit blame \"$@\" abbrev.t >actual &&\n+\t\tperl -lne \"/[0-9a-f]+/ and print \\$&\" <actual >actual.sha &&\n+\t\ttest_cmp expect actual.sha\n+\t}\n+'\n+\n+test_expect_success 'blame --abbrev=<n> works' '\n+\t# non-boundary commits get +1 for alignment\n+\tcheck_abbrev 31 --abbrev=30 HEAD &&\n+\tcheck_abbrev 30 --abbrev=30 ^HEAD\n+'\n+\n+test_expect_success 'blame -l aligns regular and boundary commits' '\n+\tcheck_abbrev 40 -l HEAD &&\n+\tcheck_abbrev 39 -l ^HEAD\n+'\n+\n+test_expect_success 'blame --abbrev=40 behaves like -l' '\n+\tcheck_abbrev 40 --abbrev=40 HEAD &&\n+\tcheck_abbrev 39 --abbrev=40 ^HEAD\n+'\n+\n test_done\n-- \n2.11.0.519.g31435224cf\n\n"},{"id":"308841","messageId":"20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net","threadId":"44820","inReplyTo":"20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net","subject":"[PATCH 3/3] blame: output porcelain \"previous\" header for each file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-06T04:20:51Z","receivedAt":"2017-01-06T04:20:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible for content currently found in one file to\nhave originated in two separate files, each of which may\nhave been modified in some single older commit.  The\n--porcelain output generates an incorrect \"previous\" header\nin this case, whereas --line-porcelain gets it right.  The\nproblem is that the porcelain output tries to omit repeated\ndetails of commits, and treats \"previous\" as a property of\nthe commit, when it is really a property of the blamed block\nof lines.\n\nLet's look at an example. In a case like this, you might see\nthis output from --line-porcelain:\n\n  SOME_SHA1 1 1 1\n  author ...\n  committer ...\n  previous SOME_SHA1^ file_one\n  filename file_one\n          ...some line content...\n  SOME_SHA1 2 1 1\n  author ...\n  committer ...\n  previous SOME_SHA1^ file_two\n  filename file_two\n          ...some different content....\n\nThe \"filename\" fields tell us that the two lines are from\ntwo different files. But notice that the filename also\nappears in the \"previous\" field, which tells us where to\nstart a re-blame. The second content line never appeared in\nfile_one at all, so we would obviously need to re-blame from\nfile_two (or possibly even some other file, if had just been\nrenamed to file_two in SOME_SHA1).\n\nSo far so good. Now here's what --porcelain looks like:\n\n  SOME_SHA1 1 1 1\n  author ...\n  committer ...\n  previous SOME_SHA1^ file_one\n  filename file_one\n          ...some line content...\n  SOME_SHA1 2 1 1\n  filename file_two\n          ...some different content....\n\nWe've dropped the author and committer fields from the\nsecond line, as they would just be repeats.  But we can't\nomit \"filename\", because it depends on the actual block of\nblamed lines, not just the commit. This is handled by\nemit_porcelain_details(), which will show the filename\neither if it is the first mention of the commit _or_ if the\ncommit has multiple paths in it.\n\nBut we don't give \"previous\" the same handling. It's written\ninside emit_one_suspect_detail(), which bails early if we've\nalready seen that commit. And so the output above is wrong;\na reader would assume that the correct place to re-blame\nline two is from file_one, but that's obviously nonsense.\n\nLet's treat \"previous\" the same as \"filename\", and show it\nfresh whenever we know we are in a confusing case like this.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm assuming that the parent sha1 for a \"previous\" field will always be\nthe same for a given commit. So we really only need to care about\nreprinting when we know there are multiple paths, as this patch does\n(i.e., treat it exactly the same as \"filename\"). If I'm wrong, then\nthere's probably another corner case that this doesn't handle. I\ncouldn't think of a way to trigger such a setup, though.\n\n builtin/blame.c             |  23 +++++----\n t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 131 insertions(+), 9 deletions(-)\n create mode 100755 t/t8011-blame-split-file.sh\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex c6170fed81..3aae19a0f9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1700,13 +1700,23 @@ static void get_commit_info(struct commit *commit,\n }\n \n /*\n+ * Write out any suspect information which depends on the path. This must be\n+ * handled separately from emit_one_suspect_detail(), because a given commit\n+ * may have changes in multiple paths. So this needs to appear each time\n+ * we mention a new group.\n+ *\n  * To allow LF and other nonportable characters in pathnames,\n  * they are c-style quoted as needed.\n  */\n-static void write_filename_info(const char *path)\n+static void write_filename_info(struct origin *suspect)\n {\n+\tif (suspect->previous) {\n+\t\tstruct origin *prev = suspect->previous;\n+\t\tprintf(\"previous %s \", oid_to_hex(&prev->commit->object.oid));\n+\t\twrite_name_quoted(prev->path, stdout, '\\n');\n+\t}\n \tprintf(\"filename \");\n-\twrite_name_quoted(path, stdout, '\\n');\n+\twrite_name_quoted(suspect->path, stdout, '\\n');\n }\n \n /*\n@@ -1735,11 +1745,6 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n \tprintf(\"summary %s\\n\", ci.summary.buf);\n \tif (suspect->commit->object.flags & UNINTERESTING)\n \t\tprintf(\"boundary\\n\");\n-\tif (suspect->previous) {\n-\t\tstruct origin *prev = suspect->previous;\n-\t\tprintf(\"previous %s \", oid_to_hex(&prev->commit->object.oid));\n-\t\twrite_name_quoted(prev->path, stdout, '\\n');\n-\t}\n \n \tcommit_info_destroy(&ci);\n \n@@ -1760,7 +1765,7 @@ static void found_guilty_entry(struct blame_entry *ent,\n \t\t       oid_to_hex(&suspect->commit->object.oid),\n \t\t       ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n \t\temit_one_suspect_detail(suspect, 0);\n-\t\twrite_filename_info(suspect->path);\n+\t\twrite_filename_info(suspect);\n \t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n \tpi->blamed_lines += ent->num_lines;\n@@ -1884,7 +1889,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)\n {\n \tif (emit_one_suspect_detail(suspect, repeat) ||\n \t    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))\n-\t\twrite_filename_info(suspect->path);\n+\t\twrite_filename_info(suspect);\n }\n \n static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,\ndiff --git a/t/t8011-blame-split-file.sh b/t/t8011-blame-split-file.sh\nnew file mode 100755\nindex 0000000000..831125047b\n--- /dev/null\n+++ b/t/t8011-blame-split-file.sh\n@@ -0,0 +1,117 @@\n+#!/bin/sh\n+\n+test_description='\n+The general idea is that we have a single file whose lines come from\n+multiple other files, and those individual files were modified in the same\n+commits. That means that we will see the same commit in multiple contexts,\n+and each one should be attributed to the correct file.\n+\n+Note that we need to use \"blame -C\" to find the commit for all lines. We will\n+not bother testing that the non-C case fails to find it. That is how blame\n+behaves now, but it is not a property we want to make sure is retained.\n+'\n+. ./test-lib.sh\n+\n+# help avoid typing and reading long strings of similar lines\n+# in the tests below\n+generate_expect () {\n+\twhile read nr data\n+\tdo\n+\t\ti=0\n+\t\twhile test $i -lt $nr\n+\t\tdo\n+\t\t\techo $data\n+\t\t\ti=$((i + 1))\n+\t\tdone\n+\tdone\n+}\n+\n+test_expect_success 'setup split file case' '\n+\t# use lines long enough to trigger content detection\n+\ttest_seq 1000 1010 >one &&\n+\ttest_seq 2000 2010 >two &&\n+\tgit add one two &&\n+\ttest_commit base &&\n+\n+\tsed \"6s/^/modified /\" <one >one.tmp &&\n+\tmv one.tmp one &&\n+\tsed \"6s/^/modified /\" <two >two.tmp &&\n+\tmv two.tmp two &&\n+\tgit add -u &&\n+\ttest_commit modified &&\n+\n+\tcat one two >combined &&\n+\tgit add combined &&\n+\tgit rm one two &&\n+\ttest_commit combined\n+'\n+\n+test_expect_success 'setup simulated porcelain' '\n+\t# This just reads porcelain-ish output and tries\n+\t# to output the value of a given field for each line (either by\n+\t# reading the field that accompanies this line, or referencing\n+\t# the information found last time the commit was mentioned).\n+\tcat >read-porcelain.pl <<-\\EOF\n+\tmy $field = shift;\n+\twhile (<>) {\n+\t\tif (/^[0-9a-f]{40} /) {\n+\t\t\tflush();\n+\t\t\t$hash = $&;\n+\t\t} elsif (/^$field (.*)/) {\n+\t\t\t$cache{$hash} = $1;\n+\t\t}\n+\t}\n+\tflush();\n+\n+\tsub flush {\n+\t\treturn unless defined $hash;\n+\t\tif (defined $cache{$hash}) {\n+\t\t\tprint \"$cache{$hash}\\n\";\n+\t\t} else {\n+\t\t\tprint \"NONE\\n\";\n+\t\t}\n+\t}\n+\tEOF\n+'\n+\n+for output in porcelain line-porcelain\n+do\n+\ttest_expect_success \"generate --$output output\" '\n+\t\tgit blame --root -C --$output combined >output\n+\t'\n+\n+\ttest_expect_success \"$output output finds correct commits\" '\n+\t\tgenerate_expect >expect <<-\\EOF &&\n+\t\t5 base\n+\t\t1 modified\n+\t\t10 base\n+\t\t1 modified\n+\t\t5 base\n+\t\tEOF\n+\t\tperl read-porcelain.pl summary <output >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$output output shows correct filenames\" '\n+\t\tgenerate_expect >expect <<-\\EOF &&\n+\t\t11 one\n+\t\t11 two\n+\t\tEOF\n+\t\tperl read-porcelain.pl filename <output >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$output output shows correct previous pointer\" '\n+\t\tgenerate_expect >expect <<-EOF &&\n+\t\t5 NONE\n+\t\t1 $(git rev-parse modified^) one\n+\t\t10 NONE\n+\t\t1 $(git rev-parse modified^) two\n+\t\t5 NONE\n+\t\tEOF\n+\t\tperl read-porcelain.pl previous <output >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+test_done\n-- \n2.11.0.519.g31435224cf\n"},{"id":"308949","messageId":"xmqqh95a1aq0.fsf@gitster.mtv.corp.google.com","threadId":"44820","inReplyTo":"20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fixing some random blame corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-08T03:39:35Z","receivedAt":"2017-01-08T03:39:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   [1/3]: blame: fix alignment with --abbrev=40\n>   [2/3]: blame: handle --no-abbrev\n>   [3/3]: blame: output porcelain \"previous\" header for each file\n\nThanks. 1 & 2 obviously look correct.  I'd need to look at 3 when I\nam not exhausted, even though I expect it to be also fine from a\ncursory read.\n\n\n\n"},{"id":"309070","messageId":"xmqqmvezuc9u.fsf@gitster.mtv.corp.google.com","threadId":"44820","inReplyTo":"20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] blame: output porcelain \"previous\" header for each file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T21:57:49Z","receivedAt":"2017-01-09T21:57:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  /*\n> + * Write out any suspect information which depends on the path. This must be\n> + * handled separately from emit_one_suspect_detail(), because a given commit\n> + * may have changes in multiple paths. So this needs to appear each time\n> + * we mention a new group.\n> + *\n>   * To allow LF and other nonportable characters in pathnames,\n>   * they are c-style quoted as needed.\n>   */\n> -static void write_filename_info(const char *path)\n> +static void write_filename_info(struct origin *suspect)\n>  {\n> +\tif (suspect->previous) {\n> +\t\tstruct origin *prev = suspect->previous;\n> +\t\tprintf(\"previous %s \", oid_to_hex(&prev->commit->object.oid));\n> +\t\twrite_name_quoted(prev->path, stdout, '\\n');\n> +\t}\n>  \tprintf(\"filename \");\n> -\twrite_name_quoted(path, stdout, '\\n');\n> +\twrite_name_quoted(suspect->path, stdout, '\\n');\n>  }\n\nYes, \"previous\" is not per commit but per \"origin\", and \"origin\" is\n(commit, path) pair, so allowing this helper to examine the entire\nsuspect instead of just suspect->path makes sense.\n\nThanks for a fix.\n"},{"id":"315533","messageId":"CACBZZX5AnG=hpnZCG3qFkLOO0gbgvt79ZYP3-oG5VjE8p284cQ@mail.gmail.com","threadId":"44820","inReplyTo":"20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] blame: output porcelain \"previous\" header for each file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-27T21:08:08Z","receivedAt":"2017-03-27T21:09:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jan 6, 2017 at 5:20 AM, Jeff King <peff@peff.net> wrote:\n\n> +for output in porcelain line-porcelain\n> +do\n> +       test_expect_success \"generate --$output output\" '\n> +               git blame --root -C --$output combined >output\n> +       '\n> +\n\nThe --root option isn't needed here, the tests pass if it's removed.\n\nDiscovered while looking at the sorry state of our blame test suite\nvis-a-vis tests for config, no tests for blame.showroot &\nblame.blankboundary.\n\nI'll submit that eventually, but meanwhile did you mean to omit --root\nhere, or is the test broken in some other way and it /should/ need\n--root but doesn't?\n"},{"id":"315538","messageId":"20170327220429.dsfp5ytq2yddfn33@sigill.intra.peff.net","threadId":"44820","inReplyTo":"CACBZZX5AnG=hpnZCG3qFkLOO0gbgvt79ZYP3-oG5VjE8p284cQ@mail.gmail.com","subject":"Re: [PATCH 3/3] blame: output porcelain \"previous\" header for each file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-27T22:04:29Z","receivedAt":"2017-03-27T22:11:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 27, 2017 at 11:08:08PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Jan 6, 2017 at 5:20 AM, Jeff King <peff@peff.net> wrote:\n> \n> > +for output in porcelain line-porcelain\n> > +do\n> > +       test_expect_success \"generate --$output output\" '\n> > +               git blame --root -C --$output combined >output\n> > +       '\n> > +\n> \n> The --root option isn't needed here, the tests pass if it's removed.\n> \n> Discovered while looking at the sorry state of our blame test suite\n> vis-a-vis tests for config, no tests for blame.showroot &\n> blame.blankboundary.\n> \n> I'll submit that eventually, but meanwhile did you mean to omit --root\n> here, or is the test broken in some other way and it /should/ need\n> --root but doesn't?\n\nI don't think it's strictly needed, though I think it's worth keeping.\n\nThe test is making sure that some lines blame to HEAD and some to HEAD^.\nBut the latter is a root commit, and so it just becomes a boundary\ncommit without --root.\n\nYou can see the difference if you interrupt the test here and run:\n\n  git blame -C [--root] combined\n\nAnd that's what I was doing when I developed the test case. If you were\nto test the non-porcelain output, you'd need to it (to match the real\nsha-1 X, and not the boundary \"^X\").\n\nWhen the porcelain outputs are used, though, the boundary commits are\nshown as full sha1s, and just get annotated with a \"boundary\" line. As\nthe tests just look for subject, filename, and previous lines, they\ndon't notice whether the boundary marker is there or not. And so --root\ncould technically go away.\n\nWe care mostly about detecting the values for the second commit, so I\ndon't think it actually matters. But if we wanted to be thorough, we\ncould confirm that the \"boundary\" lines are as we expect (or\nalternatively, we could add an uninteresting base commit at the bottom\nto make the second one non-root).\n\n-Peff\n"}]}