Eric Sunshine [off-list ref] writes:
With the introduction of check-mailmap, it is now possible to check
.mailmap functionality directly rather than indirectly as a side-effect
of other commands (such as git-shortlog), therefore, do so.
Does this patch mean that we will now ignore future breakages in
shortlog and blame if their mailmap integration becomes buggy?
I am not convinced it is a good idea if that is what is going on.
quoted hunk
Signed-off-by: Eric Sunshine <redacted>
---
t/t4203-mailmap.sh | 133 ++++++++++++++++++-----------------------------------
1 file changed, 45 insertions(+), 88 deletions(-)
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 8645492..48a000b 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' '
'
cat >expect <<\EOF
-A U Thor (1):
- initial
-
-nick1 (1):
- second
-
+A U Thor <author@example.com>
+nick1 <bugs@company.xx>
EOF
test_expect_success 'No mailmap' '
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-Repo Guy (1):
- initial
-
-nick1 (1):
- second
-
+Repo Guy <author@example.com>
+nick1 <bugs@company.xx>
EOF
test_expect_success 'default .mailmap' '
echo "Repo Guy <author@example.com>" > .mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
# Using a mailmap file in a subdirectory of the repo here, but
# could just as well have been a file outside of the repository
cat >expect <<\EOF
-Internal Guy (1):
- second
-
-Repo Guy (1):
- initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xx>
EOF
test_expect_success 'mailmap.file set' '
mkdir -p internal_mailmap &&
echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap &&
git config mailmap.file internal_mailmap/.mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-External Guy (1):
- initial
-
-Internal Guy (1):
- second
-
+External Guy <author@example.com>
+Internal Guy <bugs@company.xx>
EOF
test_expect_success 'mailmap.file override' '
echo "External Guy <author@example.com>" >> internal_mailmap/.mailmap &&
git config mailmap.file internal_mailmap/.mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-Repo Guy (1):
- initial
-
-nick1 (1):
- second
-
+Repo Guy <author@example.com>
+nick1 <bugs@company.xx>
EOF
test_expect_success 'mailmap.file non-existent' '
rm internal_mailmap/.mailmap &&
rmdir internal_mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-Internal Guy (1):
- second
-
-Repo Guy (1):
- initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xy>
EOF
test_expect_success 'name entry after email entry' '
mkdir -p internal_mailmap &&
echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-Internal Guy (1):
- second
-
-Repo Guy (1):
- initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xy>
EOF
test_expect_success 'name entry after email entry, case-insensitive' '
mkdir -p internal_mailmap &&
echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
cat >expect <<\EOF
-A U Thor (1):
- initial
-
-nick1 (1):
- second
-
+A U Thor <author@example.com>
+nick1 <bugs@company.xx>
EOF
test_expect_success 'No mailmap files, but configured' '
rm -f .mailmap internal_mailmap/.mailmap &&
- git shortlog HEAD >actual &&
+ git check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
@@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' '
test_expect_success 'mailmap.blob set' '
cat >expect <<-\EOF &&
- Blob Guy (1):
- second
-
- Repo Guy (1):
- initial
-
+ Repo Guy <author@example.com>
+ Blob Guy <bugs@company.xx>
EOF
- git -c mailmap.blob=map:just-bugs shortlog HEAD >actual &&
+ git -c mailmap.blob=map:just-bugs check-mailmap --stdin \
+ <contacts >actual &&
test_cmp expect actual
'
test_expect_success 'mailmap.blob overrides .mailmap' '
cat >expect <<-\EOF &&
- Blob Guy (2):
- initial
- second
-
+ Blob Guy <author@example.com>
+ Blob Guy <bugs@company.xx>
EOF
- git -c mailmap.blob=map:both shortlog HEAD >actual &&
+ git -c mailmap.blob=map:both check-mailmap --stdin \
+ <contacts >actual &&
test_cmp expect actual
'
test_expect_success 'mailmap.file overrides mailmap.blob' '
cat >expect <<-\EOF &&
- Blob Guy (1):
- second
-
- Internal Guy (1):
- initial
-
+ Internal Guy <author@example.com>
+ Blob Guy <bugs@company.xx>
EOF
git \
-c mailmap.blob=map:both \
-c mailmap.file=internal.map \
- shortlog HEAD >actual &&
+ check-mailmap --stdin <contacts >actual &&
test_cmp expect actual
'
test_expect_success 'mailmap.blob can be missing' '
cat >expect <<-\EOF &&
- Repo Guy (1):
- initial
-
- nick1 (1):
- second
-
+ Repo Guy <author@example.com>
+ nick1 <bugs@company.xx>
EOF
- git -c mailmap.blob=map:nonexistent shortlog HEAD >actual &&
+ git -c mailmap.blob=map:nonexistent check-mailmap --stdin \
+ <contacts >actual &&
test_cmp expect actual
'
@@ -273,12 +230,12 @@ test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
(
cd non-bare &&
test_commit one .mailmap "Fake Name <author@example.com>" &&
- echo " 1 Fake Name" >expect &&
- git shortlog -ns HEAD >actual &&
+ echo "Fake Name <author@example.com>" >expect &&
+ git check-mailmap "A U Thor <author@example.com>" >actual &&
test_cmp expect actual &&
rm .mailmap &&
- echo " 1 A U Thor" >expect &&
- git shortlog -ns HEAD >actual &&
+ echo "A U Thor <author@example.com>" >expect &&
+ git check-mailmap "A U Thor <author@example.com>" >actual &&
test_cmp expect actual
)
'
@@ -287,8 +244,8 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '
git clone --bare non-bare bare &&
(
cd bare &&
- echo " 1 Fake Name" >expect &&
- git shortlog -ns HEAD >actual &&
+ echo "Fake Name <author@example.com>" >expect &&
+ git check-mailmap "A U Thor <author@example.com>" >actual &&
test_cmp expect actual
)
'
On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano [off-list ref] wrote:
Eric Sunshine [off-list ref] writes:
quoted
With the introduction of check-mailmap, it is now possible to check
.mailmap functionality directly rather than indirectly as a side-effect
of other commands (such as git-shortlog), therefore, do so.
Does this patch mean that we will now ignore future breakages in
shortlog and blame if their mailmap integration becomes buggy?
I am not convinced it is a good idea if that is what is going on.
I meant to mention in the cover letter that this patch was open for
debate, however, it does not eliminate all testing of these other
commands.
The tests in which git-check-mailmap is substituted for git-shortlog
all worked against a simplistic two-commit repository. Those tests
were checking the low-level mailmap functionality under various
conditions and configurations; they were not especially checking any
particular behavior of git-shortlog.
There still remain a final eight tests which cover git-shortlog,
git-log, and git-blame. These tests do check mailmap-related behavior
of those commands, and they do so using a more involved seven-commit
repository with "complex" mappings, so we have not necessarily lost
any checks of mailmap integration for those commands.
Would this patch become more palatable if I stated the above in the
commit message?
-- ES
Eric Sunshine wrote:
On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano [off-list ref] wrote:
quoted
Eric Sunshine [off-list ref] writes:
quoted
quoted
With the introduction of check-mailmap, it is now possible to check
.mailmap functionality directly rather than indirectly as a side-effect
of other commands (such as git-shortlog), therefore, do so.
Does this patch mean that we will now ignore future breakages in
shortlog and blame if their mailmap integration becomes buggy?
I am not convinced it is a good idea if that is what is going on.
I meant to mention in the cover letter that this patch was open for
debate, however, it does not eliminate all testing of these other
commands.
The tests in which git-check-mailmap is substituted for git-shortlog
all worked against a simplistic two-commit repository. Those tests
were checking the low-level mailmap functionality under various
conditions and configurations; they were not especially checking any
particular behavior of git-shortlog.
There still remain a final eight tests which cover git-shortlog,
git-log, and git-blame. These tests do check mailmap-related behavior
of those commands, and they do so using a more involved seven-commit
repository with "complex" mappings, so we have not necessarily lost
any checks of mailmap integration for those commands.
Would this patch become more palatable if I stated the above in the
commit message?
My current thinking is "no" --- the patch has as a justification "Now
we can test these aspects of .mailmap handling directly with a
low-level tool instead of using the tool most people will use, so do
so", which sounds an awful lot like "Reduce test coverage of commonly
used tools, because we can".
But I imagine the actual motivation was something other than "because
we can". For example, maybe the idea is that after this patch, it
should be easier to make cosmetic improvements to the shortlog, log,
and blame output and only have to change those final 8 tests that are
adequately covering the output? If you have such plans and this patch
makes them easier, it could sound like a good patch as a stepping
stone toward that.
Thanks and hope that helps,
Jonathan
On Thu, Jul 11, 2013 at 8:55 PM, Jonathan Nieder [off-list ref] wrote:
Eric Sunshine wrote:
quoted
On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano [off-list ref] wrote:
quoted
Eric Sunshine [off-list ref] writes:
quoted
quoted
quoted
With the introduction of check-mailmap, it is now possible to check
.mailmap functionality directly rather than indirectly as a side-effect
of other commands (such as git-shortlog), therefore, do so.
Does this patch mean that we will now ignore future breakages in
shortlog and blame if their mailmap integration becomes buggy?
I am not convinced it is a good idea if that is what is going on.
I meant to mention in the cover letter that this patch was open for
debate, however, it does not eliminate all testing of these other
commands.
The tests in which git-check-mailmap is substituted for git-shortlog
all worked against a simplistic two-commit repository. Those tests
were checking the low-level mailmap functionality under various
conditions and configurations; they were not especially checking any
particular behavior of git-shortlog.
There still remain a final eight tests which cover git-shortlog,
git-log, and git-blame. These tests do check mailmap-related behavior
of those commands, and they do so using a more involved seven-commit
repository with "complex" mappings, so we have not necessarily lost
any checks of mailmap integration for those commands.
Would this patch become more palatable if I stated the above in the
commit message?
My current thinking is "no" --- the patch has as a justification "Now
we can test these aspects of .mailmap handling directly with a
low-level tool instead of using the tool most people will use, so do
so", which sounds an awful lot like "Reduce test coverage of commonly
used tools, because we can".
But I imagine the actual motivation was something other than "because
we can".
The motivation is as stated in the subject: Direct rather than
indirect testing of mailmap functionality. The tests in question are
properly crafted to check only low-level mailmap (not git-shortlog)
functionality under various conditions and configurations, yet the
mental load on the reader is high because he has to keep in mind the
authors and commits in the repository underlying git-shortlog's
output. When testing via git-check-mailmap, mental load is low: there
is a direct correlation between the "Name <email@address>" which goes
in and that which comes out. What is being tested is the same, but
it's easier to reason about and understand the the tests when done
directly.
When converting the tests, I also discovered an additional, perhaps
more important motivation. Most of the tests check only that name
remapping functions correctly, however, a couple also attempt to check
address remapping (bugs@company.xx => bugs@company.xy). Even though
the tests succeed, they don't actually prove that address remapping
was correct (or occurred at all) since git-shortlog does not display
the email address. git-check-mailmap always displays the email
address, thus the check actually tests the intended email address
remapping.
For example, maybe the idea is that after this patch, it
should be easier to make cosmetic improvements to the shortlog, log,
and blame output and only have to change those final 8 tests that are
adequately covering the output? If you have such plans and this patch
makes them easier, it could sound like a good patch as a stepping
stone toward that.