Re: [PATCH/WIP v3 06/31] am: detect mbox patches

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2015, 21:02 UTC
Message-ID: <xmqqy4jgd92l.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1434626743-8552-7-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 10 quoted lines

> +static int is_email(const char *filename)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	FILE *fp = xfopen(filename, "r");
> +	int ret = 1;
> +
> +	while (!strbuf_getline(&sb, fp, '\n')) {
> +		const char *x;
> +
> +		strbuf_rtrim(&sb);

Is this a good thing? strbuf_getline() already has stripped the LF at the end, so you'd be treating a line with only whitespaces as if it is a truly empty line.

I know the series is about literal translation and the script may lose the distinction between the two, but I do not think you need (or want) to be literally same for things like this.

Same comment applies to other uses of "trim" in this patch.

Show 14 quoted lines

> @@ -177,6 +267,14 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,
>  static void am_setup(struct am_state *state, enum patch_format patch_format,
>  		struct string_list *paths)
>  {
> +	if (!patch_format)
> +		patch_format = detect_patch_format(paths);
> +
> +	if (!patch_format) {
> +		fprintf_ln(stderr, _("Patch format detection failed."));
> +		exit(128);
> +	}
> +
>  	if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
>  		die_errno(_("failed to create directory '%s'"), state->dir.buf);

I really like the way this keeps building incrementally ;-) The series is an enjoyable read.