git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH] shortlog: remove unused(?) "repo-abbrev" feature
Date: Tue, 5 Jan 2021 22:15:35 +0100	[thread overview]
Message-ID: <CAN0heSoKYWXqskCR=GPreSHc6twCSo1345WTmiPdrR57XSShhA@mail.gmail.com> (raw)
In-Reply-To: <20210105130359.21139-1-avarab@gmail.com>

Hi Ævar,

On Tue, 5 Jan 2021 at 14:32, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>  {
>         char *name1 = NULL, *email1 = NULL, *name2 = NULL, *email2 = NULL;
> -       if (buffer[0] == '#') {
> -               static const char abbrev[] = "# repo-abbrev:";
> -               int abblen = sizeof(abbrev) - 1;
> -               int len = strlen(buffer);
>
> -               if (!repo_abbrev)
> -                       return;
> -
> -               if (len && buffer[len - 1] == '\n')
> -                       buffer[--len] = 0;
> -               if (!strncmp(buffer, abbrev, abblen)) {
> -                       char *cp;
> -
> -                       free(*repo_abbrev);
> -
> -                       for (cp = buffer + abblen; isspace(*cp); cp++)
> -                               ; /* nothing */
> -                       *repo_abbrev = xstrdup(cp);
> -               }
> -               return;
> -       }
>         if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0)) != NULL)
>                 parse_name_and_email(name2, &name2, &email2, 1);

I think this is a tiny bit too aggressive -- it stops recognizing and
skipping comments. For example, this whitespace-damaged diff:

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 586c3a86b1..4d461ad343 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -373,6 +373,7 @@ test_expect_success 'Shortlog output (complex mapping)' '
        echo "Committed <$GIT_COMMITTER_EMAIL>" > internal_mailmap/.mailmap &&
        echo "<cto@company.xx>
<cto@coompany.xx>" >> internal_mailmap/.mailmap &&
        echo "Some Dude <some@dude.xx>         nick1
<bugs@company.xx>" >> internal_mailmap/.mailmap &&
+       echo "# Comment <no@mail.xx>         nick1 <bugs@company.xx>"
>> internal_mailmap/.mailmap &&
        echo "Other Author <other@author.xx>   nick2
<bugs@company.xx>" >> internal_mailmap/.mailmap &&
        echo "Other Author <other@author.xx>
<nick2@company.xx>" >> internal_mailmap/.mailmap &&
        echo "Santa Claus <santa.claus@northpole.xx> <me@company.xx>"
>> internal_mailmap/.mailmap &&

... which passes before this, makes the test fail after this patch. It
seems our test coverage for comments is basically zero here. It might
make sense to first introduce some testing around comments (maybe not in
this "complex mapping" test, though) before doing this patch you're
posting here, but keeping something like

       if (buffer[0] == '#')
               return;

Martin

  parent reply	other threads:[~2021-01-05 21:17 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-11-25  8:11 [PATCH 2/2] git-shortlog: make common repository prefix configurable with .mailmap Junio C Hamano
2021-01-05 13:03 ` [PATCH] shortlog: remove unused(?) "repo-abbrev" feature Ævar Arnfjörð Bjarmason
2021-01-05 19:18   ` Linus Torvalds
2021-01-05 21:15   ` Martin Ågren [this message]
2021-01-05 23:06   ` Junio C Hamano
2021-01-12 20:17   ` [PATCH 00/22] mailmap: doc + test fixes Ævar Arnfjörð Bjarmason
2021-01-12 22:34     ` Junio C Hamano
2021-01-14  8:59       ` Ævar Arnfjörð Bjarmason
2021-01-14 23:02     ` [PATCH 0/2] mailmap: test cleanup Denton Liu
2021-01-14 23:02       ` [PATCH 1/2] test-lib-functions.sh: fix usage for test_commit() Denton Liu
2021-01-15  0:19         ` Ævar Arnfjörð Bjarmason
2021-01-14 23:02       ` [PATCH 2/2] t4203: stop losing return codes of git commands Denton Liu
2021-01-15  0:20         ` Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 01/22] mailmap doc: create a new "gitmailmap(5)" man page Ævar Arnfjörð Bjarmason
2021-01-14 17:41     ` Philippe Blain
2021-01-14 19:58       ` Junio C Hamano
2021-01-15  2:38         ` Philippe Blain
2021-01-15  3:18           ` Junio C Hamano
2021-01-15  3:36             ` Philippe Blain
2021-01-15  5:53               ` Junio C Hamano
2021-01-15  2:34     ` [PATCH] fixup! " Philippe Blain
2021-01-15  3:28       ` [PATCH v2] mailmap doc: use correct environment variable 'GIT_WORK_TREE' Philippe Blain
2021-01-12 20:17   ` [PATCH 02/22] mailmap doc: quote config variables `like.this` Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 03/22] check-mailmap doc: note config options Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 04/22] mailmap doc: start by mentioning the comment syntax Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 05/22] mailmap tests: use our preferred whitespace syntax Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 06/22] mailmap tests: modernize syntax & test idioms Ævar Arnfjörð Bjarmason
2021-01-14  7:51     ` Denton Liu
2021-01-14 20:00       ` Junio C Hamano
2021-01-12 20:17   ` [PATCH 07/22] mailmap tests: improve --stdin tests Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 08/22] mailmap tests: remove redundant entry in test Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 09/22] mailmap tests: add a test for "not a blob" error Ævar Arnfjörð Bjarmason
2021-01-12 22:34     ` Junio C Hamano
2021-01-12 20:17   ` [PATCH 10/22] mailmap tests: get rid of overly complex blame fuzzing Ævar Arnfjörð Bjarmason
2021-01-12 22:34     ` Junio C Hamano
2021-01-14 20:21       ` Junio C Hamano
2021-01-12 20:17   ` [PATCH 11/22] mailmap: test for silent exiting on missing file/blob Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 12/22] test-lib functions: expand "test_commit" comment template Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 13/22] test-lib functions: document arguments to test_commit Ævar Arnfjörð Bjarmason
2021-01-12 20:17   ` [PATCH 14/22] test-lib functions: add --author support " Ævar Arnfjörð Bjarmason
2021-01-12 22:34     ` Junio C Hamano
2021-01-14  7:40     ` Denton Liu
2021-01-12 20:17   ` [PATCH 15/22] test-lib functions: add an --append option " Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 16/22] tests: refactor a few tests to use "test_commit --append" Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 17/22] mailmap doc + tests: add better examples & test them Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 18/22] mailmap tests: add a test for comment syntax Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 19/22] mailmap tests: add tests for whitespace syntax Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 20/22] mailmap tests: add tests for empty "<>" syntax Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 21/22] mailmap doc + tests: document and test for case-insensitivity Ævar Arnfjörð Bjarmason
2021-01-12 20:18   ` [PATCH 22/22] shortlog: remove unused(?) "repo-abbrev" feature Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAN0heSoKYWXqskCR=GPreSHc6twCSo1345WTmiPdrR57XSShhA@mail.gmail.com' \
    --to=martin.agren@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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).