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

[Marius Storm-Olsen <marius@trolltech.com>]

[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]

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.

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


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


>> +#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 :-)


>> @@ -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;
...
>> +		} 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'


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]