{"thread":{"id":"59631","subject":"[PATCH] blame: use different author name for fake commit generated by --contents","startedAt":"2023-04-21T22:30:29Z","lastAt":"2023-04-24T18:43:18Z","messageCount":6,"participants":["Jacob Keller","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475823","messageId":"20230421223013.467142-1-jacob.e.keller@intel.com","threadId":"59631","inReplyTo":null,"subject":"[PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2023-04-21T22:30:13Z","receivedAt":"2023-04-21T22:30:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nWhen the --contents option is used with git blame, and the contents of\nthe file have lines which can't be annotated by the history being\nblamed, the user will see an author of \"Not Committed Yet\". This is\nsimilar to the way blame handles working tree contents when blaming\nwithout a revision.\n\nThis is slightly confusing since this data isn't the working copy and\nwhile it is technically \"not committed yet\", its also coming from an\nexternal file. Replace this author name with \"External file\n(--contents)\" to better differentiate such lines from actual working\ncopy lines.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSuggested-by: Glen Choo <chooglen@google.com>\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/blame-options.txt |  8 +++-----\n blame.c                         |  8 ++++++--\n t/annotate-tests.sh             | 11 +++++++++--\n 3 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 95599bd6e5f4..552dcc60f2a4 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -64,11 +64,9 @@ include::line-range-format.txt[]\n \tmanual page.\n \n --contents <file>::\n-\tPretend the file being annotated has a commit with the\n-\tcontents from the named file and a parent of <rev>,\n-\tdefaulting to HEAD when no <rev> is specified. You may\n-\tspecify '-' to make the command read from the standard\n-\tinput for the file contents.\n+\tAnnotate using the contents from the named file, starting from <rev>\n+\tif it is specified, and HEAD otherwise. You may specify '-' to make\n+\tthe command read from the standard input for the file contents.\n \n --date <format>::\n \tSpecifies the format used to output dates. If --date is not\ndiff --git a/blame.c b/blame.c\nindex 2c427bcdbfdd..47dd77d045dc 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -206,8 +206,12 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n \n \torigin = make_origin(commit, path);\n \n-\tident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n-\t\t\tWANT_BLANK_IDENT, NULL, 0);\n+\tif (contents_from)\n+\t\tident = fmt_ident(\"External file (--contents)\", \"external.file\",\n+\t\t\t\t  WANT_BLANK_IDENT, NULL, 0);\n+\telse\n+\t\tident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n+\t\t\t\t  WANT_BLANK_IDENT, NULL, 0);\n \tstrbuf_addstr(&msg, \"tree 0000000000000000000000000000000000000000\\n\");\n \tfor (parent = commit->parents; parent; parent = parent->next)\n \t\tstrbuf_addf(&msg, \"parent %s\\n\",\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex b35be20cf327..859693949b94 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -72,6 +72,13 @@ test_expect_success 'blame 1 author' '\n \tcheck_count A 2\n '\n \n+test_expect_success 'blame working copy' '\n+\ttest_when_finished \"git restore file\" &&\n+\techo \"1A quick brown fox jumps over\" >file &&\n+\techo \"another lazy dog\" >> file &&\n+\tcheck_count A 1 \"Not Committed Yet\" 1\n+'\n+\n test_expect_success 'blame with --contents' '\n \tcheck_count --contents=file A 2\n '\n@@ -79,7 +86,7 @@ test_expect_success 'blame with --contents' '\n test_expect_success 'blame with --contents changed' '\n \techo \"1A quick brown fox jumps over the\" >contents &&\n \techo \"another lazy dog\" >>contents &&\n-\tcheck_count --contents=contents A 1 \"Not Committed Yet\" 1\n+\tcheck_count --contents=contents A 1 \"External file (--contents)\" 1\n '\n \n test_expect_success 'blame in a bare repo without starting commit' '\n@@ -109,7 +116,7 @@ test_expect_success 'blame 2 authors' '\n '\n \n test_expect_success 'blame with --contents and revision' '\n-\tcheck_count -h testTag --contents=file A 2 \"Not Committed Yet\" 2\n+\tcheck_count -h testTag --contents=file A 2 \"External file (--contents)\" 2\n '\n \n test_expect_success 'setup B1 lines (branch1)' '\n-- \n2.40.0.471.gbd7f14d9353b\n\n"},{"id":"475824","messageId":"xmqqbkjgizcv.fsf@gitster.g","threadId":"59631","inReplyTo":"20230421223013.467142-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-21T23:34:24Z","receivedAt":"2023-04-21T23:34:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> +test_expect_success 'blame working copy' '\n> +\ttest_when_finished \"git restore file\" &&\n> +\techo \"1A quick brown fox jumps over\" >file &&\n> +\techo \"another lazy dog\" >> file &&\n\nLose the SP between \">>\" redirection operator and its operand\n\"file\".\n\nSo, we have \"1A quick brown fox jumps over the\" and \"lazy dog\"\nin :file and HEAD:file, and both of these lines are different\nin the working tree files as shown above.\n\n> +\tcheck_count A 1 \"Not Committed Yet\" 1\n\nSo why do we expect one is attributed to A while the other is\nattributed to the working tree file?  Shouldn't we be expecting both\nto be attributed to \"Not Committed Yet\"?\n\nWIth this updated like the attached, 8001, 8002, and 8012 seem to\nall pass (and without, they all fail).\n\n t/annotate-tests.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/t/annotate-tests.sh w/t/annotate-tests.sh\nindex 859693949b..4238ce45f8 100644\n--- c/t/annotate-tests.sh\n+++ w/t/annotate-tests.sh\n@@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '\n \n test_expect_success 'blame working copy' '\n \ttest_when_finished \"git restore file\" &&\n-\techo \"1A quick brown fox jumps over\" >file &&\n-\techo \"another lazy dog\" >> file &&\n+\techo \"11A quick brown fox jumps over the\" >file &&\n+\techo \"lazy dog\" >>file &&\n \tcheck_count A 1 \"Not Committed Yet\" 1\n '\n \n\n\n"},{"id":"475825","messageId":"019057ab-c917-80cd-063b-4871e47dc382@intel.com","threadId":"59631","inReplyTo":"xmqqbkjgizcv.fsf@gitster.g","subject":"Re: [PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2023-04-22T00:11:54Z","receivedAt":"2023-04-22T00:12:12Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 4/21/2023 4:34 PM, Junio C Hamano wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>> +test_expect_success 'blame working copy' '\n>> +\ttest_when_finished \"git restore file\" &&\n>> +\techo \"1A quick brown fox jumps over\" >file &&\n>> +\techo \"another lazy dog\" >> file &&\n> \n> Lose the SP between \">>\" redirection operator and its operand\n> \"file\".\n> \n\nYep.\n\n> So, we have \"1A quick brown fox jumps over the\" and \"lazy dog\"\n> in :file and HEAD:file, and both of these lines are different\n> in the working tree files as shown above.\n> \n>> +\tcheck_count A 1 \"Not Committed Yet\" 1\n> \n> So why do we expect one is attributed to A while the other is\n> attributed to the working tree file?  Shouldn't we be expecting both\n> to be attributed to \"Not Committed Yet\"?\n> \n> WIth this updated like the attached, 8001, 8002, and 8012 seem to\n> all pass (and without, they all fail).\n> \n\nI think I just missed a \"the\".\n\n>  t/annotate-tests.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh\n> index 859693949b..4238ce45f8 100644\n> --- c/t/annotate-tests.sh\n> +++ w/t/annotate-tests.sh\n> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '\n>  \n>  test_expect_success 'blame working copy' '\n>  \ttest_when_finished \"git restore file\" &&\n> -\techo \"1A quick brown fox jumps over\" >file &&\n> -\techo \"another lazy dog\" >> file &&\n> +\techo \"11A quick brown fox jumps over the\" >file &&\n> +\techo \"lazy dog\" >>file &&\n\nI think the right fix for this test is to keep the first line (1A) the\nsame, and include the missing \"the\" I had removed before, and keep the\n2nd line as the changed line with \"another lazy dog\".\n\nWill fix in v2, and double check the tests. I had run them but my local\nsystem sometimes fails the following test:\n\n> not ok 46 - passing hostname resolution information works\n> #\n> #               BOGUS_HOST=gitbogusexamplehost.invalid &&\n> #               BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\n> #               test_must_fail git ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null &&\n> #               git -c \"http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1\" ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null\n> #\n\nI had thought this was the only failure, and that it has something to do\nwith my system configuration (possibly proxy settings) which affect\nthis.. I checked the firewall configuration and it doesn't appear to be\nthat...\n\nIt would be nice to figure out what makes it so the tests fail so that I\ncan make sure tests properly pass on my submissions before sending them\nin the future.\n\nThanks,\nJake\n\n\n>  \tcheck_count A 1 \"Not Committed Yet\" 1\n>  '\n>  \n> \n> \n"},{"id":"475928","messageId":"xmqqh6t5gt9b.fsf@gitster.g","threadId":"59631","inReplyTo":"019057ab-c917-80cd-063b-4871e47dc382@intel.com","subject":"Re: [PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T16:05:52Z","receivedAt":"2023-04-24T16:05:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n>>  test_expect_success 'blame working copy' '\n>>  \ttest_when_finished \"git restore file\" &&\n>> -\techo \"1A quick brown fox jumps over\" >file &&\n>> -\techo \"another lazy dog\" >> file &&\n>> +\techo \"11A quick brown fox jumps over the\" >file &&\n>> +\techo \"lazy dog\" >>file &&\n>\n> I think the right fix for this test is to keep the first line (1A) the\n> same, and include the missing \"the\" I had removed before, and keep the\n> 2nd line as the changed line with \"another lazy dog\".\n\nEither is fine as long as one line is left intact and the other line\nis modified, as the end result we want is for one to be attributed\nto the floating change while the other one to be attributed to the\nHEAD.\n\n>\n> Will fix in v2, and double check the tests. \n\nThanks.\n\n> I had run them but my local\n> system sometimes fails the following test:\n\nHmph, I do not recall seeing t5551 fail but we had seen a fair\nnumber of flaky http tests, and I would not be surprised if there\nwere still remaining flaky tests there.\n\n>\n>> not ok 46 - passing hostname resolution information works\n>> #\n>> #               BOGUS_HOST=gitbogusexamplehost.invalid &&\n>> #               BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\n>> #               test_must_fail git ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null &&\n>> #               git -c \"http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1\" ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null\n>> #\n>\n> I had thought this was the only failure, and that it has something to do\n> with my system configuration (possibly proxy settings) which affect\n> this.. I checked the firewall configuration and it doesn't appear to be\n> that...\n>\n> It would be nice to figure out what makes it so the tests fail so that I\n> can make sure tests properly pass on my submissions before sending them\n> in the future.\n>\n> Thanks,\n> Jake\n>\n>\n>>  \tcheck_count A 1 \"Not Committed Yet\" 1\n>>  '\n>>  \n>> \n>> \n"},{"id":"475952","messageId":"kl6l354p9n52.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59631","inReplyTo":"019057ab-c917-80cd-063b-4871e47dc382@intel.com","subject":"Re: [PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-24T17:59:53Z","receivedAt":"2023-04-24T18:00:01Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n>> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh\n>> index 859693949b..4238ce45f8 100644\n>> --- c/t/annotate-tests.sh\n>> +++ w/t/annotate-tests.sh\n>> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '\n>>  \n>>  test_expect_success 'blame working copy' '\n>>  \ttest_when_finished \"git restore file\" &&\n>> -\techo \"1A quick brown fox jumps over\" >file &&\n>> -\techo \"another lazy dog\" >> file &&\n>> +\techo \"11A quick brown fox jumps over the\" >file &&\n>> +\techo \"lazy dog\" >>file &&\n>\n> I think the right fix for this test is to keep the first line (1A) the\n> same, and include the missing \"the\" I had removed before, and keep the\n> 2nd line as the changed line with \"another lazy dog\".\n\nThis sounds right to me; it's easier to read when the working copy test\nand the --contents test use the same data\n>\n>> not ok 46 - passing hostname resolution information works\n>> #\n>> #               BOGUS_HOST=gitbogusexamplehost.invalid &&\n>> #               BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\n>> #               test_must_fail git ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null &&\n>> #               git -c \"http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1\" ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null\n>> #\n>\n> I had thought this was the only failure, and that it has something to do\n> with my system configuration (possibly proxy settings) which affect\n> this.. I checked the firewall configuration and it doesn't appear to be\n> that...\n>\n> It would be nice to figure out what makes it so the tests fail so that I\n> can make sure tests properly pass on my submissions before sending them\n> in the future.\n\nI remember seeing a similar, flaky failure on an older version of master\n(~2-3 months ago). But if you based this on a recent version, I'm afraid\nI haven't seen this :/\n"},{"id":"475958","messageId":"90f8499b-891f-ec32-096c-9ae532ef65c2@intel.com","threadId":"59631","inReplyTo":"kl6l354p9n52.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] blame: use different author name for fake commit generated by --contents","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2023-04-24T18:37:20Z","receivedAt":"2023-04-24T18:43:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 4/24/2023 10:59 AM, Glen Choo wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>>> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh\n>>> index 859693949b..4238ce45f8 100644\n>>> --- c/t/annotate-tests.sh\n>>> +++ w/t/annotate-tests.sh\n>>> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '\n>>>  \n>>>  test_expect_success 'blame working copy' '\n>>>  \ttest_when_finished \"git restore file\" &&\n>>> -\techo \"1A quick brown fox jumps over\" >file &&\n>>> -\techo \"another lazy dog\" >> file &&\n>>> +\techo \"11A quick brown fox jumps over the\" >file &&\n>>> +\techo \"lazy dog\" >>file &&\n>>\n>> I think the right fix for this test is to keep the first line (1A) the\n>> same, and include the missing \"the\" I had removed before, and keep the\n>> 2nd line as the changed line with \"another lazy dog\".\n> \n> This sounds right to me; it's easier to read when the working copy test\n> and the --contents test use the same data\n\nYep, will have it this way in v2.\n\n>>\n>>> not ok 46 - passing hostname resolution information works\n>>> #\n>>> #               BOGUS_HOST=gitbogusexamplehost.invalid &&\n>>> #               BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\n>>> #               test_must_fail git ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null &&\n>>> #               git -c \"http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1\" ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null\n>>> #\n>>\n>> I had thought this was the only failure, and that it has something to do\n>> with my system configuration (possibly proxy settings) which affect\n>> this.. I checked the firewall configuration and it doesn't appear to be\n>> that...\n>>\n>> It would be nice to figure out what makes it so the tests fail so that I\n>> can make sure tests properly pass on my submissions before sending them\n>> in the future.\n> \n> I remember seeing a similar, flaky failure on an older version of master\n> (~2-3 months ago). But if you based this on a recent version, I'm afraid\n> I haven't seen this :/\n\nI'm running on 667fcf4e1537 (\"The tenth batch\", 2023-04-17) and the\nfailure is consistent.\n\nI'm not familiar with the curlopt stuff but it does seem like some sort\nof environment failure..\n\nWhen running with -x and -v:\n\n> ++ git -c http.curloptResolve=gitbogusexamplehost.invalid:5551:127.0.0.1 ls-remote http://gitbogusexamplehost.invalid:5551/smart/repo.git\n> fatal: unable to access 'http://gitbogusexamplehost.invalid:5551/smart/repo.git/': The requested URL returned error: 503\n> error: last command exited with $?=128\n\nIt looks like this is a straight wrapper around CURLOPT_RESOLVE. It\nlooks like the HTTPD server starts just fine but perhaps some firewall\nconfiguration is blocking connections.\n\nI found I can skip the tests by setting GIT_TEST_HTTPD=no so thats a\ndecent workaround for me for now.\n\nThanks,\nJake\n"}]}