* [PATCH] blame: use different author name for fake commit generated by --contents
@ 2023-04-21 22:30 Jacob Keller
2023-04-21 23:34 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Jacob Keller @ 2023-04-21 22:30 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Junio C Hamano, Glen Choo
From: Jacob Keller <jacob.keller@gmail.com>
When the --contents option is used with git blame, and the contents of
the file have lines which can't be annotated by the history being
blamed, the user will see an author of "Not Committed Yet". This is
similar to the way blame handles working tree contents when blaming
without a revision.
This is slightly confusing since this data isn't the working copy and
while it is technically "not committed yet", its also coming from an
external file. Replace this author name with "External file
(--contents)" to better differentiate such lines from actual working
copy lines.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/blame-options.txt | 8 +++-----
blame.c | 8 ++++++--
t/annotate-tests.sh | 11 +++++++++--
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 95599bd6e5f4..552dcc60f2a4 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -64,11 +64,9 @@ include::line-range-format.txt[]
manual page.
--contents <file>::
- Pretend the file being annotated has a commit with the
- contents from the named file and a parent of <rev>,
- defaulting to HEAD when no <rev> is specified. You may
- specify '-' to make the command read from the standard
- input for the file contents.
+ Annotate using the contents from the named file, starting from <rev>
+ if it is specified, and HEAD otherwise. You may specify '-' to make
+ the command read from the standard input for the file contents.
--date <format>::
Specifies the format used to output dates. If --date is not
diff --git a/blame.c b/blame.c
index 2c427bcdbfdd..47dd77d045dc 100644
--- a/blame.c
+++ b/blame.c
@@ -206,8 +206,12 @@ static struct commit *fake_working_tree_commit(struct repository *r,
origin = make_origin(commit, path);
- ident = fmt_ident("Not Committed Yet", "not.committed.yet",
- WANT_BLANK_IDENT, NULL, 0);
+ if (contents_from)
+ ident = fmt_ident("External file (--contents)", "external.file",
+ WANT_BLANK_IDENT, NULL, 0);
+ else
+ ident = fmt_ident("Not Committed Yet", "not.committed.yet",
+ WANT_BLANK_IDENT, NULL, 0);
strbuf_addstr(&msg, "tree 0000000000000000000000000000000000000000\n");
for (parent = commit->parents; parent; parent = parent->next)
strbuf_addf(&msg, "parent %s\n",
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index b35be20cf327..859693949b94 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -72,6 +72,13 @@ test_expect_success 'blame 1 author' '
check_count A 2
'
+test_expect_success 'blame working copy' '
+ test_when_finished "git restore file" &&
+ echo "1A quick brown fox jumps over" >file &&
+ echo "another lazy dog" >> file &&
+ check_count A 1 "Not Committed Yet" 1
+'
+
test_expect_success 'blame with --contents' '
check_count --contents=file A 2
'
@@ -79,7 +86,7 @@ test_expect_success 'blame with --contents' '
test_expect_success 'blame with --contents changed' '
echo "1A quick brown fox jumps over the" >contents &&
echo "another lazy dog" >>contents &&
- check_count --contents=contents A 1 "Not Committed Yet" 1
+ check_count --contents=contents A 1 "External file (--contents)" 1
'
test_expect_success 'blame in a bare repo without starting commit' '
@@ -109,7 +116,7 @@ test_expect_success 'blame 2 authors' '
'
test_expect_success 'blame with --contents and revision' '
- check_count -h testTag --contents=file A 2 "Not Committed Yet" 2
+ check_count -h testTag --contents=file A 2 "External file (--contents)" 2
'
test_expect_success 'setup B1 lines (branch1)' '
--
2.40.0.471.gbd7f14d9353b
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-21 22:30 Jacob Keller
@ 2023-04-21 23:34 ` Junio C Hamano
2023-04-22 0:11 ` Jacob Keller
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2023-04-21 23:34 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller, Glen Choo
Jacob Keller <jacob.e.keller@intel.com> writes:
> +test_expect_success 'blame working copy' '
> + test_when_finished "git restore file" &&
> + echo "1A quick brown fox jumps over" >file &&
> + echo "another lazy dog" >> file &&
Lose the SP between ">>" redirection operator and its operand
"file".
So, we have "1A quick brown fox jumps over the" and "lazy dog"
in :file and HEAD:file, and both of these lines are different
in the working tree files as shown above.
> + check_count A 1 "Not Committed Yet" 1
So why do we expect one is attributed to A while the other is
attributed to the working tree file? Shouldn't we be expecting both
to be attributed to "Not Committed Yet"?
WIth this updated like the attached, 8001, 8002, and 8012 seem to
all pass (and without, they all fail).
t/annotate-tests.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh
index 859693949b..4238ce45f8 100644
--- c/t/annotate-tests.sh
+++ w/t/annotate-tests.sh
@@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '
test_expect_success 'blame working copy' '
test_when_finished "git restore file" &&
- echo "1A quick brown fox jumps over" >file &&
- echo "another lazy dog" >> file &&
+ echo "11A quick brown fox jumps over the" >file &&
+ echo "lazy dog" >>file &&
check_count A 1 "Not Committed Yet" 1
'
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-21 23:34 ` Junio C Hamano
@ 2023-04-22 0:11 ` Jacob Keller
2023-04-24 16:05 ` Junio C Hamano
2023-04-24 17:59 ` Glen Choo
0 siblings, 2 replies; 8+ messages in thread
From: Jacob Keller @ 2023-04-22 0:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jacob Keller, Glen Choo
On 4/21/2023 4:34 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> +test_expect_success 'blame working copy' '
>> + test_when_finished "git restore file" &&
>> + echo "1A quick brown fox jumps over" >file &&
>> + echo "another lazy dog" >> file &&
>
> Lose the SP between ">>" redirection operator and its operand
> "file".
>
Yep.
> So, we have "1A quick brown fox jumps over the" and "lazy dog"
> in :file and HEAD:file, and both of these lines are different
> in the working tree files as shown above.
>
>> + check_count A 1 "Not Committed Yet" 1
>
> So why do we expect one is attributed to A while the other is
> attributed to the working tree file? Shouldn't we be expecting both
> to be attributed to "Not Committed Yet"?
>
> WIth this updated like the attached, 8001, 8002, and 8012 seem to
> all pass (and without, they all fail).
>
I think I just missed a "the".
> t/annotate-tests.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh
> index 859693949b..4238ce45f8 100644
> --- c/t/annotate-tests.sh
> +++ w/t/annotate-tests.sh
> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '
>
> test_expect_success 'blame working copy' '
> test_when_finished "git restore file" &&
> - echo "1A quick brown fox jumps over" >file &&
> - echo "another lazy dog" >> file &&
> + echo "11A quick brown fox jumps over the" >file &&
> + echo "lazy dog" >>file &&
I think the right fix for this test is to keep the first line (1A) the
same, and include the missing "the" I had removed before, and keep the
2nd line as the changed line with "another lazy dog".
Will fix in v2, and double check the tests. I had run them but my local
system sometimes fails the following test:
> not ok 46 - passing hostname resolution information works
> #
> # BOGUS_HOST=gitbogusexamplehost.invalid &&
> # BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&
> # test_must_fail git ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null &&
> # git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
> #
I had thought this was the only failure, and that it has something to do
with my system configuration (possibly proxy settings) which affect
this.. I checked the firewall configuration and it doesn't appear to be
that...
It would be nice to figure out what makes it so the tests fail so that I
can make sure tests properly pass on my submissions before sending them
in the future.
Thanks,
Jake
> check_count A 1 "Not Committed Yet" 1
> '
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-22 0:11 ` Jacob Keller
@ 2023-04-24 16:05 ` Junio C Hamano
2023-04-24 17:59 ` Glen Choo
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2023-04-24 16:05 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller, Glen Choo
Jacob Keller <jacob.e.keller@intel.com> writes:
>> test_expect_success 'blame working copy' '
>> test_when_finished "git restore file" &&
>> - echo "1A quick brown fox jumps over" >file &&
>> - echo "another lazy dog" >> file &&
>> + echo "11A quick brown fox jumps over the" >file &&
>> + echo "lazy dog" >>file &&
>
> I think the right fix for this test is to keep the first line (1A) the
> same, and include the missing "the" I had removed before, and keep the
> 2nd line as the changed line with "another lazy dog".
Either is fine as long as one line is left intact and the other line
is modified, as the end result we want is for one to be attributed
to the floating change while the other one to be attributed to the
HEAD.
>
> Will fix in v2, and double check the tests.
Thanks.
> I had run them but my local
> system sometimes fails the following test:
Hmph, I do not recall seeing t5551 fail but we had seen a fair
number of flaky http tests, and I would not be surprised if there
were still remaining flaky tests there.
>
>> not ok 46 - passing hostname resolution information works
>> #
>> # BOGUS_HOST=gitbogusexamplehost.invalid &&
>> # BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&
>> # test_must_fail git ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null &&
>> # git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
>> #
>
> I had thought this was the only failure, and that it has something to do
> with my system configuration (possibly proxy settings) which affect
> this.. I checked the firewall configuration and it doesn't appear to be
> that...
>
> It would be nice to figure out what makes it so the tests fail so that I
> can make sure tests properly pass on my submissions before sending them
> in the future.
>
> Thanks,
> Jake
>
>
>> check_count A 1 "Not Committed Yet" 1
>> '
>>
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-22 0:11 ` Jacob Keller
2023-04-24 16:05 ` Junio C Hamano
@ 2023-04-24 17:59 ` Glen Choo
2023-04-24 18:37 ` Jacob Keller
1 sibling, 1 reply; 8+ messages in thread
From: Glen Choo @ 2023-04-24 17:59 UTC (permalink / raw)
To: Jacob Keller, Junio C Hamano; +Cc: git, Jacob Keller
Jacob Keller <jacob.e.keller@intel.com> writes:
>> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh
>> index 859693949b..4238ce45f8 100644
>> --- c/t/annotate-tests.sh
>> +++ w/t/annotate-tests.sh
>> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '
>>
>> test_expect_success 'blame working copy' '
>> test_when_finished "git restore file" &&
>> - echo "1A quick brown fox jumps over" >file &&
>> - echo "another lazy dog" >> file &&
>> + echo "11A quick brown fox jumps over the" >file &&
>> + echo "lazy dog" >>file &&
>
> I think the right fix for this test is to keep the first line (1A) the
> same, and include the missing "the" I had removed before, and keep the
> 2nd line as the changed line with "another lazy dog".
This sounds right to me; it's easier to read when the working copy test
and the --contents test use the same data
>
>> not ok 46 - passing hostname resolution information works
>> #
>> # BOGUS_HOST=gitbogusexamplehost.invalid &&
>> # BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&
>> # test_must_fail git ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null &&
>> # git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
>> #
>
> I had thought this was the only failure, and that it has something to do
> with my system configuration (possibly proxy settings) which affect
> this.. I checked the firewall configuration and it doesn't appear to be
> that...
>
> It would be nice to figure out what makes it so the tests fail so that I
> can make sure tests properly pass on my submissions before sending them
> in the future.
I remember seeing a similar, flaky failure on an older version of master
(~2-3 months ago). But if you based this on a recent version, I'm afraid
I haven't seen this :/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-24 17:59 ` Glen Choo
@ 2023-04-24 18:37 ` Jacob Keller
0 siblings, 0 replies; 8+ messages in thread
From: Jacob Keller @ 2023-04-24 18:37 UTC (permalink / raw)
To: Glen Choo, Junio C Hamano; +Cc: git, Jacob Keller
On 4/24/2023 10:59 AM, Glen Choo wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>>> diff --git c/t/annotate-tests.sh w/t/annotate-tests.sh
>>> index 859693949b..4238ce45f8 100644
>>> --- c/t/annotate-tests.sh
>>> +++ w/t/annotate-tests.sh
>>> @@ -74,8 +74,8 @@ test_expect_success 'blame 1 author' '
>>>
>>> test_expect_success 'blame working copy' '
>>> test_when_finished "git restore file" &&
>>> - echo "1A quick brown fox jumps over" >file &&
>>> - echo "another lazy dog" >> file &&
>>> + echo "11A quick brown fox jumps over the" >file &&
>>> + echo "lazy dog" >>file &&
>>
>> I think the right fix for this test is to keep the first line (1A) the
>> same, and include the missing "the" I had removed before, and keep the
>> 2nd line as the changed line with "another lazy dog".
>
> This sounds right to me; it's easier to read when the working copy test
> and the --contents test use the same data
Yep, will have it this way in v2.
>>
>>> not ok 46 - passing hostname resolution information works
>>> #
>>> # BOGUS_HOST=gitbogusexamplehost.invalid &&
>>> # BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&
>>> # test_must_fail git ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null &&
>>> # git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
>>> #
>>
>> I had thought this was the only failure, and that it has something to do
>> with my system configuration (possibly proxy settings) which affect
>> this.. I checked the firewall configuration and it doesn't appear to be
>> that...
>>
>> It would be nice to figure out what makes it so the tests fail so that I
>> can make sure tests properly pass on my submissions before sending them
>> in the future.
>
> I remember seeing a similar, flaky failure on an older version of master
> (~2-3 months ago). But if you based this on a recent version, I'm afraid
> I haven't seen this :/
I'm running on 667fcf4e1537 ("The tenth batch", 2023-04-17) and the
failure is consistent.
I'm not familiar with the curlopt stuff but it does seem like some sort
of environment failure..
When running with -x and -v:
> ++ git -c http.curloptResolve=gitbogusexamplehost.invalid:5551:127.0.0.1 ls-remote http://gitbogusexamplehost.invalid:5551/smart/repo.git
> fatal: unable to access 'http://gitbogusexamplehost.invalid:5551/smart/repo.git/': The requested URL returned error: 503
> error: last command exited with $?=128
It looks like this is a straight wrapper around CURLOPT_RESOLVE. It
looks like the HTTPD server starts just fine but perhaps some firewall
configuration is blocking connections.
I found I can skip the tests by setting GIT_TEST_HTTPD=no so thats a
decent workaround for me for now.
Thanks,
Jake
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] blame: use different author name for fake commit generated by --contents
@ 2023-04-24 19:35 Jacob Keller
2023-04-24 19:36 ` Jacob Keller
0 siblings, 1 reply; 8+ messages in thread
From: Jacob Keller @ 2023-04-24 19:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller, Glen Choo
From: Jacob Keller <jacob.keller@gmail.com>
When the --contents option is used with git blame, and the contents of
the file have lines which can't be annotated by the history being
blamed, the user will see an author of "Not Committed Yet". This is
similar to the way blame handles working tree contents when blaming
without a revision.
This is slightly confusing since this data isn't the working copy and
while it is technically "not committed yet", its also coming from an
external file. Replace this author name with "External file
(--contents)" to better differentiate such lines from actual working
copy lines.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes since v2
* Fix the test case to pass and align with the other similar test cases
Documentation/blame-options.txt | 8 +++-----
blame.c | 8 ++++++--
t/annotate-tests.sh | 11 +++++++++--
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 95599bd6e5f4..552dcc60f2a4 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -64,11 +64,9 @@ include::line-range-format.txt[]
manual page.
--contents <file>::
- Pretend the file being annotated has a commit with the
- contents from the named file and a parent of <rev>,
- defaulting to HEAD when no <rev> is specified. You may
- specify '-' to make the command read from the standard
- input for the file contents.
+ Annotate using the contents from the named file, starting from <rev>
+ if it is specified, and HEAD otherwise. You may specify '-' to make
+ the command read from the standard input for the file contents.
--date <format>::
Specifies the format used to output dates. If --date is not
diff --git a/blame.c b/blame.c
index 2c427bcdbfdd..47dd77d045dc 100644
--- a/blame.c
+++ b/blame.c
@@ -206,8 +206,12 @@ static struct commit *fake_working_tree_commit(struct repository *r,
origin = make_origin(commit, path);
- ident = fmt_ident("Not Committed Yet", "not.committed.yet",
- WANT_BLANK_IDENT, NULL, 0);
+ if (contents_from)
+ ident = fmt_ident("External file (--contents)", "external.file",
+ WANT_BLANK_IDENT, NULL, 0);
+ else
+ ident = fmt_ident("Not Committed Yet", "not.committed.yet",
+ WANT_BLANK_IDENT, NULL, 0);
strbuf_addstr(&msg, "tree 0000000000000000000000000000000000000000\n");
for (parent = commit->parents; parent; parent = parent->next)
strbuf_addf(&msg, "parent %s\n",
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index b35be20cf327..2ef70235b101 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -72,6 +72,13 @@ test_expect_success 'blame 1 author' '
check_count A 2
'
+test_expect_success 'blame working copy' '
+ test_when_finished "git restore file" &&
+ echo "1A quick brown fox jumps over the" >file &&
+ echo "another lazy dog" >>file &&
+ check_count A 1 "Not Committed Yet" 1
+'
+
test_expect_success 'blame with --contents' '
check_count --contents=file A 2
'
@@ -79,7 +86,7 @@ test_expect_success 'blame with --contents' '
test_expect_success 'blame with --contents changed' '
echo "1A quick brown fox jumps over the" >contents &&
echo "another lazy dog" >>contents &&
- check_count --contents=contents A 1 "Not Committed Yet" 1
+ check_count --contents=contents A 1 "External file (--contents)" 1
'
test_expect_success 'blame in a bare repo without starting commit' '
@@ -109,7 +116,7 @@ test_expect_success 'blame 2 authors' '
'
test_expect_success 'blame with --contents and revision' '
- check_count -h testTag --contents=file A 2 "Not Committed Yet" 2
+ check_count -h testTag --contents=file A 2 "External file (--contents)" 2
'
test_expect_success 'setup B1 lines (branch1)' '
--
2.40.0.471.gbd7f14d9353b
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] blame: use different author name for fake commit generated by --contents
2023-04-24 19:35 [PATCH] blame: use different author name for fake commit generated by --contents Jacob Keller
@ 2023-04-24 19:36 ` Jacob Keller
0 siblings, 0 replies; 8+ messages in thread
From: Jacob Keller @ 2023-04-24 19:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller, Glen Choo
On 4/24/2023 12:35 PM, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
Apologies but I forgot to add -v2 when formatting this patch, so the
subject really should be [PATCH v2].
Thanks,
Jake
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-24 19:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-24 19:35 [PATCH] blame: use different author name for fake commit generated by --contents Jacob Keller
2023-04-24 19:36 ` Jacob Keller
-- strict thread matches above, loose matches on Subject: below --
2023-04-21 22:30 Jacob Keller
2023-04-21 23:34 ` Junio C Hamano
2023-04-22 0:11 ` Jacob Keller
2023-04-24 16:05 ` Junio C Hamano
2023-04-24 17:59 ` Glen Choo
2023-04-24 18:37 ` Jacob Keller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).