git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Alexander Kuleshov <kuleshovmail@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] format-patch: introduce format.outputDirectory configuration
Date: Thu, 18 Jun 2015 15:57:51 -0400	[thread overview]
Message-ID: <20150618195751.GA14550@peff.net> (raw)
In-Reply-To: <xmqq616ley7y.fsf@gitster.dls.corp.google.com>

On Thu, Jun 18, 2015 at 10:13:37AM -0700, Junio C Hamano wrote:

> > -static const char *set_outdir(const char *prefix, const char *output_directory)
> > +static const char *set_outdir(const char *prefix, const char *output_directory,
> > +			      const char *config_output_directory)
> 
> This change looks ugly and unnecessary.  All the machinery after and
> including the point set_outdir() is called, including reopen_stdout(),
> work on output_directory variable and only that variable.
> 
> Wouldn't it work equally well to have
> 
> 	if (!output_directory)
>         	output_directory = config_output_directory;
> 
> before a call to set_outdir() is made but after the configuration is
> read (namely, soon after parse_options() returns), without making
> any change to this function?

Don't we load the config before parsing options here? In that case, we
can use our usual strategy to just set output_directory (which is
already a static global) from the config callback, and everything Just
Works.

We do have to bump the definition of output_directory up above the
config callback, like so (while we are here, we might also want to
drop the unnecessary static initializers, which violate our style guide):

diff --git a/builtin/log.c b/builtin/log.c
index e67671e..77c06f7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -37,6 +37,10 @@ static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
+static FILE *realstdout = NULL;
+static const char *output_directory = NULL;
+static int outdir_offset;
+
 static const char * const builtin_log_usage[] = {
 	N_("git log [<options>] [<revision-range>] [[--] <path>...]"),
 	N_("git show [<options>] <object>..."),
@@ -752,14 +756,12 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
 		return 0;
 	}
+	if (!strcmp(var, "format.outputdirectory"))
+		return git_config_string(&output_directory, var, value);
 
 	return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
-static const char *output_directory = NULL;
-static int outdir_offset;
-
 static int reopen_stdout(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {

  reply	other threads:[~2015-06-18 19:58 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-18 11:18 [PATCH] format-patch: introduce format.outputDirectory configuration Alexander Kuleshov
2015-06-18 17:13 ` Junio C Hamano
2015-06-18 19:57   ` Jeff King [this message]
2015-06-18 20:05     ` Junio C Hamano
2015-06-18 20:06       ` Junio C Hamano
2015-06-18 20:13         ` Jeff King
2015-06-18 20:22           ` Jeff King
2015-06-18 21:46             ` Junio C Hamano
2015-06-19  4:14               ` Jeff King
2015-06-19  7:06                 ` Alexander Kuleshov
2015-06-19 13:33               ` Alexander Kuleshov
2015-06-19 15:59                 ` Junio C Hamano
2015-06-19 17:19                   ` Alexander Kuleshov
2015-06-19 17:27                     ` Alexander Kuleshov
2015-06-19 17:49                       ` Alexander Kuleshov
2015-06-19 18:14                       ` Junio C Hamano
2015-06-19 11:34 ` Remi Galan Alfonso
2015-06-19 11:34   ` Alexander Kuleshov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150618195751.GA14550@peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kuleshovmail@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).