# [PATCH 0/4] Microfixes to mailmap 14 messages from 2013-07-12 to 2013-07-15. Participants: Junio C Hamano, Eric Sunshine, Stefan Beller. Thread: https://gitlist.dev/t/34423 ## Junio C Hamano, 2013-07-12 21:38 Subject: [PATCH 0/4] Microfixes to mailmap Message-ID: <1373665135-32484-1-git-send-email-gitster@pobox.com> URL: https://gitlist.dev/e/1373665135-32484-1-git-send-email-gitster%40pobox.com ``` There were a few bugs in mailmap parsing and matching code. Junio C Hamano (3): mailmap: do not lose single-letter names mailmap: do not downcase mailmap entries mailmap: style fixes Stefan Beller (1): Add a testcase for checking case insensitivity of mailmap mailmap.c | 37 +++++++++++++++++++------------------ t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 18 deletions(-) -- 1.8.3.2-941-gda9c3c8 ``` ## Junio C Hamano, 2013-07-12 21:38 Subject: [PATCH 1/4] mailmap: do not lose single-letter names Message-ID: <1373665135-32484-2-git-send-email-gitster@pobox.com> URL: https://gitlist.dev/e/1373665135-32484-2-git-send-email-gitster%40pobox.com In-Reply-To: <1373665135-32484-1-git-send-email-gitster@pobox.com> ``` In parse_name_and_email() function, there is this line: *name = (nstart < nend ? nstart : NULL); When the function is given a buffer "A ", nstart scans from the beginning of the buffer, skipping whitespaces (there isn't any, so nstart points at the buffer), while nend starts from one byte before the first '<' and skips whitespaces backwards and stops at the first non-whitespace (i.e. it hits "A" at the beginning of the buffer). nstart == nend in this case for a single-letter name, and an off-by-one error makes it fail to pick up the name, which makes the entry equivalent to without the name. Signed-off-by: Junio C Hamano --- mailmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mailmap.c b/mailmap.c index 2a7b366..418081e 100644 --- a/mailmap.c +++ b/mailmap.c @@ -122,7 +122,7 @@ static char *parse_name_and_email(char *buffer, char **name, while (nend > nstart && isspace(*nend)) --nend; - *name = (nstart < nend ? nstart : NULL); + *name = (nstart <= nend ? nstart : NULL); *email = left+1; *(nend+1) = '\0'; *right++ = '\0'; -- 1.8.3.2-941-gda9c3c8 ``` ## Junio C Hamano, 2013-07-12 21:38 Subject: [PATCH 2/4] mailmap: do not downcase mailmap entries Message-ID: <1373665135-32484-3-git-send-email-gitster@pobox.com> URL: https://gitlist.dev/e/1373665135-32484-3-git-send-email-gitster%40pobox.com In-Reply-To: <1373665135-32484-1-git-send-email-gitster@pobox.com> ``` The email addresses in the records read from the .mailmap file are downcased very early, and then used to match against e-mail addresses in the input. Because we do use case insensitive version of string list to manage these entries, there is no need to do this, and worse yet, downcasing the rewritten/canonical e-mail read from the .mailmap file loses information. Stop doing that, and also make the string list used to keep multiple names for an mailmap entry case insensitive (the code that uses the list, lookup_prefix(), expects a case insensitive match). Signed-off-by: Junio C Hamano --- mailmap.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/mailmap.c b/mailmap.c index 418081e..a7e92db 100644 --- a/mailmap.c +++ b/mailmap.c @@ -51,14 +51,6 @@ static void add_mapping(struct string_list *map, { struct mailmap_entry *me; int index; - char *p; - - if (old_email) - for (p = old_email; *p; p++) - *p = tolower(*p); - if (new_email) - for (p = new_email; *p; p++) - *p = tolower(*p); if (old_email == NULL) { old_email = new_email; @@ -68,13 +60,17 @@ static void add_mapping(struct string_list *map, if ((index = string_list_find_insert_index(map, old_email, 1)) < 0) { /* mailmap entry exists, invert index value */ index = -1 - index; + me = (struct mailmap_entry *)map->items[index].util; } else { /* create mailmap entry */ - struct string_list_item *item = string_list_insert_at_index(map, index, old_email); - item->util = xcalloc(1, sizeof(struct mailmap_entry)); - ((struct mailmap_entry *)item->util)->namemap.strdup_strings = 1; + struct string_list_item *item; + + item = string_list_insert_at_index(map, index, old_email); + me = xcalloc(1, sizeof(struct mailmap_entry)); + me->namemap.strdup_strings = 1; + me->namemap.cmp = strcasecmp; + item->util = me; } - me = (struct mailmap_entry *)map->items[index].util; if (old_name == NULL) { debug_mm("mailmap: adding (simple) entry for %s at index %d\n", old_email, index); -- 1.8.3.2-941-gda9c3c8 ``` ## Junio C Hamano, 2013-07-12 21:38 Subject: [PATCH 3/4] mailmap: style fixes Message-ID: <1373665135-32484-4-git-send-email-gitster@pobox.com> URL: https://gitlist.dev/e/1373665135-32484-4-git-send-email-gitster%40pobox.com In-Reply-To: <1373665135-32484-1-git-send-email-gitster@pobox.com> ``` Wrap overlong lines and format the multi-line comments to match our coding style. Signed-off-by: Junio C Hamano --- mailmap.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/mailmap.c b/mailmap.c index a7e92db..5a3347d 100644 --- a/mailmap.c +++ b/mailmap.c @@ -37,7 +37,8 @@ static void free_mailmap_info(void *p, const char *s) static void free_mailmap_entry(void *p, const char *s) { struct mailmap_entry *me = (struct mailmap_entry *)p; - debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n", s, me->namemap.nr); + debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n", + s, me->namemap.nr); debug_mm("mailmap: - simple: '%s' <%s>\n", me->name, me->email); free(me->name); free(me->email); @@ -73,7 +74,8 @@ static void add_mapping(struct string_list *map, } if (old_name == NULL) { - debug_mm("mailmap: adding (simple) entry for %s at index %d\n", old_email, index); + debug_mm("mailmap: adding (simple) entry for %s at index %d\n", + old_email, index); /* Replace current name and new email for simple entry */ if (new_name) { free(me->name); @@ -85,7 +87,8 @@ static void add_mapping(struct string_list *map, } } else { struct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info)); - debug_mm("mailmap: adding (complex) entry for %s at index %d\n", old_email, index); + debug_mm("mailmap: adding (complex) entry for %s at index %d\n", + old_email, index); if (new_name) mi->name = xstrdup(new_name); if (new_email) @@ -315,8 +318,10 @@ int map_user(struct string_list *map, if (item != NULL) { me = (struct mailmap_entry *)item->util; if (me->namemap.nr) { - /* The item has multiple items, so we'll look up on name too */ - /* If the name is not found, we choose the simple entry */ + /* + * The item has multiple items, so we'll look up on name too. + * If the name is not found, we choose the simple entry. + */ struct string_list_item *subitem; subitem = lookup_prefix(&me->namemap, *name, *namelen); if (subitem) -- 1.8.3.2-941-gda9c3c8 ``` ## Junio C Hamano, 2013-07-12 21:38 Subject: [PATCH 4/4] add a testcase for checking case insensitivity of mailmap Message-ID: <1373665135-32484-5-git-send-email-gitster@pobox.com> URL: https://gitlist.dev/e/1373665135-32484-5-git-send-email-gitster%40pobox.com In-Reply-To: <1373665135-32484-1-git-send-email-gitster@pobox.com> ``` From: Stefan Beller Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 842b754..07152e9 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -409,4 +409,37 @@ test_expect_success 'Blame output (complex mapping)' ' test_cmp expect actual.fuzz ' +test_expect_success 'cleanup after mailmap.blob tests' ' + rm -f .mailmap +' + +cat >expect <<\EOF + 2 A + 2 Other Author + 2 Santa Claus + 1 A U Thor + 1 CTO + 1 Some Dude +EOF + +test_expect_success 'Test case sensitivity of Names' ' + # do a commit: + echo "asdf" > test1 + git add test1 + git commit -a --author="A " -m "add test1" + + # commit with same name, but different email + # (different capitalization does the trick already, + # but here I am going to use a different mail) + echo "asdf" > test2 + git add test2 + git commit -a --author="A " -m "add test2" + + # Adding the line to the mailmap should make life easy, so we know + # it is the same person + echo "A " > .mailmap + + git shortlog -sne HEAD >actual && test_cmp expect actual +' + test_done -- 1.8.3.2-941-gda9c3c8 ``` ## Eric Sunshine, 2013-07-12 22:38 Subject: Re: [PATCH 4/4] add a testcase for checking case insensitivity of mailmap Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTE7ELo9xfNqOCq1p7xUgOUrr9WDiRkTbXgHPjqtzeEsw%40mail.gmail.com In-Reply-To: <1373665135-32484-5-git-send-email-gitster@pobox.com> ``` On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano wrote: > From: Stefan Beller > > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 842b754..07152e9 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -409,4 +409,37 @@ test_expect_success 'Blame output (complex mapping)' ' > test_cmp expect actual.fuzz > ' > > +test_expect_success 'cleanup after mailmap.blob tests' ' > + rm -f .mailmap > +' This "test" was copied from earlier in the file, but the description was not updated; it has nothing to do with mailmap.blob tests for which cleanup has already taken place. It's also misleading since no .mailmap file exists at this point. Instead, configuration key mailmap.file is set to internal_mailmap/.mailmap, so if you need to clean up anything, it would be that. (You're also recreating .mailmap from scratch directly in your test via "echo foo >.mailmap", so this "test" doesn't really add anything.) > +cat >expect <<\EOF > + 2 A > + 2 Other Author > + 2 Santa Claus > + 1 A U Thor > + 1 CTO > + 1 Some Dude > +EOF > + > +test_expect_success 'Test case sensitivity of Names' ' s/Names/names/ Also, most of the test descriptions in this script start with lowercase, so s/Test/test/ would be appropriate. > + # do a commit: > + echo "asdf" > test1 Git practice normally avoids whitespace after the redirection operator. This particular test script, has a mix of ">foo" and "> foo", but we may want to avoid adding more of the latter form. > + git add test1 > + git commit -a --author="A " -m "add test1" > + > + # commit with same name, but different email > + # (different capitalization does the trick already, > + # but here I am going to use a different mail) Without context of the mailing list discussion, the reader does not know what "trick" is or precisely how this may have failed in the past. It's also not clear why the test is being performed with a different email address entirely rather than one which differs only by case (which is what the test description talks about). > + echo "asdf" > test2 Whitespace after redirection. > + git add test2 > + git commit -a --author="A " -m "add test2" > + > + # Adding the line to the mailmap should make life easy, so we know > + # it is the same person The comment can probably be dropped, as the new .mailmap entry is self-explanatory. > + echo "A " > .mailmap Whitespace after redirection. > + > + git shortlog -sne HEAD >actual && test_cmp expect actual For consistency with more other tests, perhaps break the line apart: git shortlog -sne HEAD >actual && test_cmp expect actual > +' > + > test_done > -- > 1.8.3.2-941-gda9c3c8 ``` ## Eric Sunshine, 2013-07-12 22:51 Subject: Re: [PATCH 4/4] add a testcase for checking case insensitivity of mailmap Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS-AGTa7DUZKsy2cq6Pd0LFV_4BBFsCVd%2BCGZ%2BxWpVh5w%40mail.gmail.com In-Reply-To: <1373665135-32484-5-git-send-email-gitster@pobox.com> ``` On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano wrote: > From: Stefan Beller > > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 842b754..07152e9 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > +test_expect_success 'Test case sensitivity of Names' ' > + # do a commit: > + echo "asdf" > test1 > + git add test1 > + git commit -a --author="A " -m "add test1" > + > + # commit with same name, but different email > + # (different capitalization does the trick already, > + # but here I am going to use a different mail) > + echo "asdf" > test2 > + git add test2 > + git commit -a --author="A " -m "add test2" > + > + # Adding the line to the mailmap should make life easy, so we know > + # it is the same person > + echo "A " > .mailmap > + > + git shortlog -sne HEAD >actual && test_cmp expect actual > +' I forgot to mention that the &&-chain is broken (missing) in the entire test (except for the last line). ``` ## Stefan Beller, 2013-07-13 07:20 Subject: [PATCH] mailmap: Testing the single letter name case. Message-ID: <1373700020-30436-1-git-send-email-stefanbeller@googlemail.com> URL: https://gitlist.dev/e/1373700020-30436-1-git-send-email-stefanbeller%40googlemail.com In-Reply-To: <1373665135-32484-2-git-send-email-gitster@pobox.com> ``` This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991 Signed-off-by: Stefan Beller --- t/t4203-mailmap.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 842b754..9ec87a2 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -409,4 +409,45 @@ test_expect_success 'Blame output (complex mapping)' ' test_cmp expect actual.fuzz ' +cat >expect <<\EOF +A (2): + eighth + nineth + +A U Thor (1): + initial + +CTO (1): + seventh + +Other Author (2): + third + fourth + +Santa Claus (2): + fifth + sixth + +Some Dude (1): + second + +EOF + +# Regression test +# Using a single letter name to check for off-by-one errors in parse_name_and_email +test_expect_success 'check mapping for short names' ' + echo one >two && + git add two && + git commit --author "A " -m "eighth" && + + echo two >> two && + git add two && + git commit --author "A " -m "nineth" && + + echo "A " >> .mailmap && + git shortlog HEAD -e >actual + + test_cmp expect actual +' + test_done -- 1.8.3.2.776.gfcf213d ``` ## Junio C Hamano, 2013-07-13 17:38 Subject: Re: [PATCH] mailmap: Testing the single letter name case. Message-ID: <7v8v1aicn5.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8v1aicn5.fsf%40alter.siamese.dyndns.org In-Reply-To: <1373700020-30436-1-git-send-email-stefanbeller@googlemail.com> ``` Stefan Beller writes: > This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991 Sorry, I do not quite get this. If you apply the patch on top of the said commit before that commit, the new test does not pass. But if you apply the patch on top of the said commit, the new test still does not pass. So I am having hard time guessing what you meant by "regression test". It is not "a66e77 broke something that ought to work, and this shows the breakage". It is not "a66e77 fixed something and this shows the previous breakage that got fixed." It may be because the test is depending on ShortName not to be downcased incorrectly, which was to be fixed by a later commit. But after applying this on top of jc/mailmap-case-insensitivity topic, the test does not pass (and reverting a66e77 does not seem to affect the result, either). ``` ## Stefan Beller, 2013-07-13 18:14 Subject: Re: [PATCH] mailmap: Testing the single letter name case. Message-ID: <51E198FC.5080005@googlemail.com> URL: https://gitlist.dev/e/51E198FC.5080005%40googlemail.com In-Reply-To: <7v8v1aicn5.fsf@alter.siamese.dyndns.org> ``` On 07/13/2013 07:38 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991 > > Sorry, I do not quite get this. > > If you apply the patch on top of the said commit before that commit, the > new test does not pass. > > But if you apply the patch on top of the said commit, the new test > still does not pass. > > So I am having hard time guessing what you meant by "regression > test". It is not "a66e77 broke something that ought to work, and > this shows the breakage". It is not "a66e77 fixed something and > this shows the previous breakage that got fixed." > > It may be because the test is depending on ShortName not to be > downcased incorrectly, which was to be fixed by a later commit. But > after applying this on top of jc/mailmap-case-insensitivity topic, > the test does not pass (and reverting a66e77 does not seem to affect > the result, either). > I am sorry for the bad wording, sorry for wasting your time. This patch was meant to replace 134d1ac9be2ce97c60a7e9187c32980681811cb5 (current test, latest commit on mailmap-case-insensitivity) Indeed the patch tests for both bugs unintentionally. So both the one letter name is being used and the email case sensitivity is checked. So maybe I should redo it again and make 2 explicit test cases for each of the fixes? Stefan ``` ## Junio C Hamano, 2013-07-13 20:20 Subject: Re: [PATCH] mailmap: Testing the single letter name case. Message-ID: <7vk3kugqkl.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk3kugqkl.fsf%40alter.siamese.dyndns.org In-Reply-To: <51E198FC.5080005@googlemail.com> ``` Stefan Beller writes: > Indeed the patch tests for both bugs unintentionally. I was puzzled because I do not think that is what is happening with the posted patch. If I drop the tip one from jc/mailmap-case-insensitivity and apply this patch instead, the test passes (which is good). git checkout 1ab5d42 git am URL: https://gitlist.dev/e/CAPig%2BcRQ5yVYjY%3D%3D5P%2BkKJycdHDp9avzcxHvZj34GJ3oBAcesg%40mail.gmail.com In-Reply-To: <1373700020-30436-1-git-send-email-stefanbeller@googlemail.com> ``` On Sat, Jul 13, 2013 at 3:20 AM, Stefan Beller wrote: > This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991 > > Signed-off-by: Stefan Beller > --- > t/t4203-mailmap.sh | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 842b754..9ec87a2 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -409,4 +409,45 @@ test_expect_success 'Blame output (complex mapping)' ' > test_cmp expect actual.fuzz > ' > > +# Regression test > +# Using a single letter name to check for off-by-one errors in parse_name_and_email > +test_expect_success 'check mapping for short names' ' > + echo one >two && > + git add two && > + git commit --author "A " -m "eighth" && > + > + echo two >> two && s/>> />>/ > + git add two && > + git commit --author "A " -m "nineth" && > + > + echo "A " >> .mailmap && s/>> />>/ > + git shortlog HEAD -e >actual Broken &&-chain: s/actual/actual &&/ > + > + test_cmp expect actual > +' > + > test_done > -- > 1.8.3.2.776.gfcf213d > ``` ## Eric Sunshine, 2013-07-15 02:31 Subject: Re: [PATCH] mailmap: Testing the single letter name case. Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR8zhcZ9_Gjq9mv0kPMe7F%3DszeSANG1b3J3gitoWCZaxA%40mail.gmail.com In-Reply-To: <7vk3kugqkl.fsf@alter.siamese.dyndns.org> ``` On Sat, Jul 13, 2013 at 4:20 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> Indeed the patch tests for both bugs unintentionally. > > I was puzzled because I do not think that is what is happening with > the posted patch. > > The off-by-one fix seems to be correct from code inspection, but the > new test does not seem to demonstrate a case where the code before > the fix misbehaves. I've written tests which check for the two bugs which Junio's patches fix. I'll be sending them along with a re-roll of Junio's series along with a few other fixes to mailmap's debug mode. ``` ## Junio C Hamano, 2013-07-15 15:54 Subject: Re: [PATCH] mailmap: Testing the single letter name case. Message-ID: <7vzjtnbyy8.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzjtnbyy8.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thanks. ```