patch, 4 partsMicrofixes to mailmap

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

In parse_name_and_email() function, there is this line:

	*name = (nstart < nend ? nstart : NULL);

When the function is given a buffer "A <A@example.org> <old@x.z>", 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

	<A@example.org> <old@x.z>

without the name.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 t/t4203-mailmap.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Stefan Beller <stefanbeller@googlemail.com> writes:

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

On 07/13/2013 07:38 PM, Junio C Hamano wrote:

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

Stefan Beller <stefanbeller@googlemail.com> writes:

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 <sb.mbox
        make && cd t && sh t4203-mailmap.sh

Of course, if I revert 543f991 (i.e. do not downcase fix), the test in this patch fails as expected.

	git checkout 1ab5d42
        git am <sb.mbox
	git show 543f991 | git apply -R
        make && cd t && sh t4203-mailmap.sh ;# should fail and does

But if I only revert a66e77e (i.e. off-by-one fix) while still keeping the downcase fix, the test in this patch should fail---but it doesn't seem to.

	git checkout 1ab5d42
        git am <sb.mbox
	git show a66e77e | git apply -R -3
        make && cd t && sh t4203-mailmap.sh ;# should fail but doesn't

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.

On Sat, Jul 13, 2013 at 4:20 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Thanks.

On Sat, Jul 13, 2013 at 3:20 AM, Stefan Beller <stefanbeller@googlemail.com> wrote:

s/>> />>/

s/>> />>/

Broken &&-chain: s/actual/actual &&/

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 <gitster@pobox.com>
---
 mailmap.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Wrap overlong lines and format the multi-line comments to match our coding style.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

From: Stefan Beller <stefanbeller@googlemail.com>

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

s/Names/names/

Also, most of the test descriptions in this script start with lowercase, so s/Test/test/ would be appropriate.

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.

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

Whitespace after redirection.

The comment can probably be dropped, as the new .mailmap entry is self-explanatory.

Whitespace after redirection.

For consistency with more other tests, perhaps break the line apart:

  git shortlog -sne HEAD >actual &&
  test_cmp expect actual

On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

I forgot to mention that the &&-chain is broken (missing) in the entire test (except for the last line).