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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano said the following on 04.02.2009 07:50:
Marius Storm-Olsen <marius@xxxxxxxxxxxxx> 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@xxxxxxxxxx>

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: signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux