Re: [PATCH 2/5] add the ability to select more email header fields to output

From: Junio C Hamano <junkio@cox.net>
Date: Mar 13, 2007, 00:55 UTC
Message-ID: <7vveh6nes9.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11737291281648-git-send-email-dzickus@redhat.com>

Don Zickus <dzickus@redhat.com> writes:

> This is useful when scripts need more than just the basic email headers to
> parse.  By specifying the "-x=" option, one can search and output any header
> field they want.

It probably is useful, but that is rather difficult to judge, unless you have a specific use in the scripts (am/applymox).

Show 17 quoted lines

> @@ -870,6 +871,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  	def_charset = (git_commit_encoding ? git_commit_encoding : "utf-8");
>  	metainfo_charset = def_charset;
>  
> +	for (top=0; header[top]; top++){ ; }
> +
>  	while (1 < argc && argv[1][0] == '-') {
>  		if (!strcmp(argv[1], "-k"))
>  			keep_subject = 1;
> @@ -879,7 +882,10 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  			metainfo_charset = NULL;
>  		else if (!prefixcmp(argv[1], "--encoding="))
>  			metainfo_charset = argv[1] + 11;
> -		else
> +		else if (!prefixcmp(argv[1], "-x=")) {
> +			header[top] = xmalloc(256*sizeof(char));
> +			strncpy(header[top++], argv[1]+3, 256);

Return "-ETOOMANYEXTRAHEADERSHEADERS" when top overflows, perhaps?

You seem to omit SP around '=' in initializers (the first part of for loop and "typename var=init" at the beginning of block) but not in an assignment expression used as a freestanding statement. Is this recommended by some coding style I am not aware of, or it it just your habit? It is somewhat irritating to my eyes, although they might be syntactically different class and you might be using one from the other consistently (but in [1/5] some SP around '=' in assignments are omitted, and there does not seem to be any such consistency).

And a micronit on [1/5] in the series. I do not think "less than zero" comment applies to what is being done, and I do not think it needs to be explained what the code is doing by checking return value from strcasestr() with NULL.

+	char boundary[256];
+
+	/* the only time this return less than zero is when 
+	   /line/ does not contain "text/"
 	 */
-	if (strcasestr(line, "boundary=")) {
-		fprintf(stderr, "Not handling nested multipart message.\n");
-		exit(1);
+	if (strcasestr(line, "text/") == NULL)
+		 message_type = TYPE_OTHER;

I think 3, 4, and 5 are good changes too. Will apply with the abovementioned micronit fixups and let's cook them in 'next'.