Re: [PATCH v6] mailinfo: allow e-mail files as input

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 17, 2009, 01:05 UTC
Message-ID: <7v1vog6rw9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <f006bbb9c754c80c133798ff70db5b5291dae060.1247766192.git.nicolas.s.dev@gmx.fr>

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> We traditionally allowed a mbox file or a directory name of a maildir to be
> ...
> Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>

Thanks.

I have one more comment on the test script, but it's something I can locally fix (iow, there is no need to resend your patch if there is no other issue pointed out by others, and if you agree to my suggested improvements).

Show 12 quoted lines

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index a12bf84..4c99240 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -63,6 +63,53 @@ with the data reset to initial values.
>  
>  EOF
>  
> +cat >rfc2822_email <<EOF
> +Return-Path: <user@domain.name>
> +X-Flags: 0000
> +	999

Please quote that EOF if you do not mean to have any $variable substituted in the "here document", like this:

	cat >rfc2822_email <<\EOF
        ... here document comes here ...
	EOF

Otherwise reviewers need to waste time looking for what is being replaced in the huge here document to understand what is going on.

Show 8 quoted lines

> +Delivered-To: delivery to user@domain.name
> +Received: (qmail invoked by alias); 16 Jul 2009 05:25:49 -0000
> +Received: from vger.knl.xyz (EHLO vger.knl.xyz) [4.3.2.1]
> +  by mx0.gmx.com (mx-us004) with SMTP; 16 Jul 2009 01:25:49 -0400
> ...
> +
> +EOF
> +

The headers look a bit too excessive to my taste, but probably you wanted to take a real-life example. If that is the case, I suspect the manually added X-Flags: at the beginning defeats that purpose, though. I'd suggest either removing the hand-munging, or triming the Received: sequence to make it a bit shorter.

Show 10 quoted lines

> @@ -222,6 +269,13 @@ test_expect_success 'am takes patches from a Pine mailbox' '
>  	test -z "$(git diff master^..HEAD)"
>  '
>  
> +test_expect_success 'am takes patches from a RFC2822 formated email' '
> +	git checkout first &&
> +	cat rfc2822_email patch1 | git am &&
> +	! test -d .git/rebase-apply &&
> +	test -z "$(git diff master^..HEAD)"
> +'

These days we tend to write the last step

	git diff --exit-code master^ HEAD

which allows "sh t4150-am.sh -i -v" to be more useful when debugging. But the existing tests in this script are old fashioned, and you matched their style in this patch, which is very good.

We probably would want a separate patch to modernize them after 1.6.4, though.