From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 18 Jun 2015 20:52:30 GMT
Subject: Re: [PATCH/WIP v3 05/31] am: split out mbox/maildir patches with git-mailsplit
Message-ID: <xmqq381oeo35.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1434626743-8552-6-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

> @@ -111,13 +122,69 @@ static void am_destroy(const struct am_state *state)
>  }
>  
>  /**
> + * Splits out individual patches from `paths`, where each path is either a mbox
> + * file or a Maildir. Return 0 on success, -1 on failure.
> + */

"Splits" and then "Return"?  Be consistent.

> +static int split_patches_mbox(struct am_state *state, struct string_list *paths)
> +{
> ...
> +}

Looks straightforward ;-)

> +/**
> + * parse_options() callback that validates and sets opt->value to the
> + * PATCH_FORMAT_* enum value corresponding to `arg`.
> + */
> +static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)
> +{
> +	int *opt_value = opt->value;
> +
> +	if (!strcmp(arg, "mbox"))
> +		*opt_value = PATCH_FORMAT_MBOX;
> +	else
> +		return -1;
> +	return 0;
> +}
> +
>  static struct am_state state;
> +static int opt_patch_format;
>  
>  static const char * const am_usage[] = {
>  	N_("git am [options] [(<mbox>|<Maildir>)...]"),
> @@ -156,6 +239,8 @@ static const char * const am_usage[] = {
>  };
>  
>  static struct option am_options[] = {
> +	OPT_CALLBACK(0, "patch-format", &opt_patch_format, N_("format"),
> +		N_("format the patch(es) are in"), parse_opt_patchformat),
>  	OPT_END()
>  };

Looking good ;-).

Just FYI, you do not have to make am_options[], and the variables
that are referenced from there, e.g. opt_patch_format, etc., global
variables (instead you can have them all in the function scope of
cmd_am()).