Re: [PATCH v3 3/4] Add map_user() and clear_mailmap() to mailmap

[Junio C Hamano <gitster@pobox.com>]

Marius Storm-Olsen <marius@trolltech.com> writes:

> map_user() allows to lookup and replace both email and
> name of a user, based on a new style mailmap file.
> The possible mailmap definitions now are:
>   proper_name <commit_email>                             // Old style
>   proper_name <proper_email> <commit_email>              // New style
>   proper_name <proper_email> commit_name <commit_email>  // New style
>
> map_email() operates the same as before, with the
> exception that it also will to try to match on a name
> passed in through the name return buffer.
>
> clear_mailmap() is needed to now clear the more complex
> mailmap structure.

Use of ample paragraph breaks would make it much easier to eyes.

    map_user() allows to lookup and replace both email and name of a user,
    based on a new style mailmap file.

    The possible mailmap definitions now are:

      proper_name <commit_email>                             # Old style
      proper_name <proper_email> <commit_email>              # New style
      proper_name <proper_email> commit_name <commit_email>  # New style

    map_email() operates the same as before, with the exception that it also
    will to try to match on a name passed in through the name return buffer.

    clear_mailmap() is needed to now clear the more complex mailmap structure.

Also, the example shows how lines would look line in a mailmap file.  I
would avoid giving a false impression that the parser should take C++
style comment introducer // by using '#' which is a documented one (I
suspect anything that follows the last angle bracket is simply ignored,
though).

> @@ -86,6 +95,27 @@ Jane Doe <jane@desktop.(none)>
>  Joe R. Developer <joe@random.com>

This context line was updated a few days ago (not a big deal, just in case
you didn't know).

> diff --git a/mailmap.c b/mailmap.c
> index 5aaee91..f593ff0 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -2,7 +2,87 @@
>  #include "string-list.h"
>  #include "mailmap.h"
>  
> +#define DEBUG_MAILMAP 0
> +#if DEBUG_MAILMAP
> +#define debug_mm(...) fprintf(stderr, __VA_ARGS__)
> +#else
> +inline void debug_mm(const char *format, ...) {}
> +#endif

"static inline void ...";

> @@ -37,25 +117,65 @@ static int read_single_mailmap(struct string_list *map, const char *filename, ch
>...
> +		/* Locate 2nd name and email. Possible mappings in mailmap file are:
> +		 *   proper_name <commit_email>
> +		 *   proper_name <proper_email> <commit_email>
> +		 *   proper_name <proper_email> commit_name <commit_email>
> +		 */

	/*
         * We tend to write a multi line comment block
         * like this.
         */

> +		do {
> +			email2 = name2 = 0;
> +			right_bracket1 += 1;
> +			if ((left_bracket2 = strchr(right_bracket1, '<')) == NULL)
> +				continue;
> +			if ((right_bracket2 = strchr(left_bracket2 + 1, '>')) == NULL)
> +				continue;
> +			if (right_bracket2 == left_bracket2 + 1)
> +				continue;
> +			for (end_of_name = left_bracket2;
> +			     end_of_name != right_bracket1 && isspace(end_of_name[-1]);
> +			     end_of_name--)
> +				; /* keep on looking for name end */
> +			for (;
> +			     end_of_name != right_bracket1 && isspace(right_bracket1[0]);
> +			     right_bracket1++)
> +				; /* keep on looking for name start */
> +			if (end_of_name != right_bracket1) {
> +				name2 = xmalloc(end_of_name - right_bracket1 + 1);
> +				strlcpy(name2, right_bracket1, end_of_name - right_bracket1 + 1);
> +				char *tmp = name1;
> +				name1 = name2;
> +				name2 = tmp;
> +			}
> +			email2 = xmalloc(right_bracket2 - left_bracket2);
> +			for (i = 0; i < right_bracket2 - left_bracket2 - 1; i++)
> +				email2[i] = tolower(left_bracket2[i + 1]);
> +			email2[right_bracket2 - left_bracket2 - 1] = '\0';
> +			char *tmp = email1;
> +			email1 = email2;
> +			email2 = tmp;
> +		} while(0);

Yuck.  Is it just me or is this new codeblock especially denser than existing
code?  I wonder use of a few smaller helper functions (that the compiler
may be able to inline without being told for us) would make this easier to read
without funny-looking "do { ... if (...) continue; } while (0)" trick?

Two "char *tmp" in this scope are both decl-after-statement errors.