From: Martin Ågren <martin.agren@gmail.com>
Date: Tue, 05 Jan 2021 21:15:35 GMT
Subject: Re: [PATCH] shortlog: remove unused(?) "repo-abbrev" feature
Message-ID: <CAN0heSoKYWXqskCR=GPreSHc6twCSo1345WTmiPdrR57XSShhA@mail.gmail.com>
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