From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 13 Jan 2016 06:52:11 GMT
Subject: Re: [PATCH v5] format-patch: introduce format.outputDirectory configuration
Message-ID: <CAPig+cTCRq9VCT7t8E9yjk4QcHYB2_qeBwGB_31keB4nTjkLkA@mail.gmail.com>
In-Reply-To: <1452660493-5430-1-git-send-email-ischis2@cox.net>

On Tue, Jan 12, 2016 at 11:48 PM, Stephen P. Smith <ischis2@cox.net> wrote:
> From: Alexander Kuleshov <kuleshovmail@gmail.com>
>
> We can pass -o/--output-directory to the format-patch command to store
> patches in some place other than the working directory. This patch
> introduces format.outputDirectory configuration option for same
> purpose.
>
> The case of usage of this configuration option can be convinience

Mentioned several times already: s/convinience/convenience/

> to not pass every time -o/--output-directory if an user has pattern
> to store all patches in the /patches directory for example.
>
> The format.outputDirectory has lower priority than command line
> option, so if user will set format.outputDirectory and pass the
> command line option, a result will be stored in a directory that
> passed to command line option.
>
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
> Notes:
>     Fixed bug which was found when moving the tests to the end of the
>     script by removing the hardcoded expected count and replacing with a
>     computation of the number of actual patches.

Thanks, this version looks better. Aside from the misspelling above
and a minor comment below, this version is:

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

>     Mailing list web interface is again not working; therefore, I don't
>     have URLs for the earlier review comments.

The full set of attempts is here [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/272180

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -707,6 +707,8 @@ enum {
>         COVER_AUTO
>  };
>
> +static const char *config_output_directory;

I don't care strongly, but I wonder why this new variable is placed
below the enum rather than being grouped with other similar variables
just above this enum. (Probably not worth a re-roll, though.)

>  static int git_format_config(const char *var, const char *value, void *cb)
>  {
>         if (!strcmp(var, "format.headers")) {