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

From: Marius Storm-Olsen <marius@trolltech.com>
Date: Feb 4, 2009, 08:50 UTC
Message-ID: <498956E9.3050606@trolltech.com>
In-Reply-To: <7v3aeu66hv.fsf@gitster.siamese.dyndns.org>

Junio C Hamano said the following on 04.02.2009 07:50:

> Marius Storm-Olsen <marius@trolltech.com> writes:
> Use of ample paragraph breaks would make it much easier to eyes.

Ok.

Show 5 quoted lines

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

Right, it is, but I agree with your point.

Show 5 quoted lines

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

Right, I saw the patch on the list, but I based the patch series on master, which I don't think had the update at the time? Anyways, do you prefer the patches based on next instead? (Documentation/SubmittingPatches says master, but maybe that has changed)

Show 7 quoted lines

>> +#if DEBUG_MAILMAP
>> +#define debug_mm(...) fprintf(stderr, __VA_ARGS__)
>> +#else
>> +inline void debug_mm(const char *format, ...) {}
>> +#endif
> 
> "static inline void ...";

Sure. (I seriously hope that the compiler optimizes that empty function call away for me though, without specifying inline :-)

Show 12 quoted lines

>> @@ -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.
>          */

Ok.

>> +		do {

...

>> +			if ((left_bracket2 = strchr(right_bracket1, '<')) == NULL)
>> +				continue;

...

Show 7 quoted lines

>> +		} 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?

Heh, It was mostly a copy'n'paste from the previous code block, with minor tweaks and variable renaming. I'll factor things out to make it an easier read.

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

Yikes! I wonder why I never got any compiler notification about those. They should never have been there, sorry.

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'