threads / patch / 39669

patchformat-patch: introduce format.outputDirectory configuration

Subject: [PATCH] format-patch: introduce format.outputDirectory configuration

## tl;dr

18 messages between Jun 18, 2015 and Jun 19, 2015. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Alexander Kuleshov· Jun 18, 2015, 11:18 UTC · lore

We can pass -o/--output-directory to the format-patch command to store patches not in the working directory. This patch introduces format.outputDirectory configuration option for same purpose.

The case of usage of this configuration option can be convinience to not pass everytime -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>
---
 Documentation/config.txt |  4 ++++
 builtin/log.c            | 14 ++++++++++++--
 t/t4014-format-patch.sh  |  9 +++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)
Show changes to 3 files +25 −2

Documentation/config.txt, builtin/log.c, t/t4014-format-patch.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fd2036c..8f6f7ed 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1247,6 +1247,10 @@ format.coverLetter::
 	format-patch is invoked, but in addition can be set to "auto", to
 	generate a cover-letter only when there's more than one patch.
 
+format.outputDirectory::
+	Set a custom directory to store the resulting files instead of the
+	current working directory.
+
 filter.<driver>.clean::
 	The command which is used to convert the content of a worktree
 	file to a blob upon checkin.  See linkgit:gitattributes[5] for
diff --git a/builtin/log.c b/builtin/log.c
index dfb351e..22c1e46 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -687,6 +687,8 @@ enum {
 	COVER_AUTO
 };
 
+static const char *config_output_directory = NULL;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -757,6 +759,9 @@ 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(&config_output_directory, var, value);
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1006,7 +1011,8 @@ static const char *clean_message_id(const char *msg_id)
 	return xmemdupz(a, z - a);
 }
 
-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)
 {
 	if (output_directory && is_absolute_path(output_directory))
 		return output_directory;
@@ -1014,6 +1020,9 @@ static const char *set_outdir(const char *prefix, const char *output_directory)
 	if (!prefix || !*prefix) {
 		if (output_directory)
 			return output_directory;
+
+		if (config_output_directory)
+			return config_output_directory;
 		/* The user did not explicitly ask for "./" */
 		outdir_offset = 2;
 		return "./";
@@ -1368,7 +1377,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		init_display_notes(&rev.notes_opt);
 
 	if (!use_stdout)
-		output_directory = set_outdir(prefix, output_directory);
+		output_directory = set_outdir(prefix, output_directory,
+					      config_output_directory);
 	else
 		setup_pager();
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c39e500..a4b18b5 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -40,6 +40,15 @@ test_expect_success setup '
 
 '
 
+test_expect_success "format-patch format.outputDirectory option" '
+	git config format.outputDirectory "patches/" &&
+	git format-patch master..side &&
+	cnt=$(ls | wc -l) &&
+	echo $cnt &&
+	test $cnt = 3 &&
+	git config --unset format.outputDirectory
+'
+
 test_expect_success "format-patch --ignore-if-in-upstream" '
 
 	git format-patch --stdout master..side >patch0 &&
-- 
2.4.0.383.gded6615.dirty
Junio C Hamano· Jun 18, 2015, 17:13 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 12 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fd2036c..8f6f7ed 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1247,6 +1247,10 @@ format.coverLetter::
>  	format-patch is invoked, but in addition can be set to "auto", to
>  	generate a cover-letter only when there's more than one patch.
>  
> +format.outputDirectory::
> +	Set a custom directory to store the resulting files instead of the
> +	current working directory.
> +

After you set this configuration variable, how would you override it and get the default behaviour back from the command line for one time invocation? "-o ./"? That needs to be documented somewhere.

Documentation/format-patch.txt must have description on -o; that paragraph needs to mention this new configuration variable, and it would be a good place to document the "-o ./" workaround.

> -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?

Jeff King· Jun 18, 2015, 19:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

On Thu, Jun 18, 2015 at 10:13:37AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> > -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):

Show changes to builtin/log.c +6 −4
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)
 {
Junio C Hamano· Jun 18, 2015, 20:05 UTC · re: Jeff King · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
>> 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):

You would also need to remove the "oh you gave me -o twice?" check, and change the semantics to "later -o overrides an earlier one", wouldn't you? Otherwise you would never be able to override what you read from the config, I am afraid.

Junio C Hamano· Jun 18, 2015, 20:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Junio C Hamano <gitster@pobox.com> writes:
Show 28 quoted lines
> Jeff King <peff@peff.net> writes:
>
>>> 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):
>
> You would also need to remove the "oh you gave me -o twice?" check,
> and change the semantics to "later -o overrides an earlier one",
> wouldn't you?  Otherwise you would never be able to override what
> you read from the config, I am afraid.

By the way, I actually think "later -o overrides an earlier one" is a good change by itself, regardless of this new configuration.

Jeff King· Jun 18, 2015, 20:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

On Thu, Jun 18, 2015 at 01:06:54PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> >> 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):
> >
> > You would also need to remove the "oh you gave me -o twice?" check,
> > and change the semantics to "later -o overrides an earlier one",
> > wouldn't you?  Otherwise you would never be able to override what
> > you read from the config, I am afraid.
> 
> By the way, I actually think "later -o overrides an earlier one" is
> a good change by itself, regardless of this new configuration.

Ah, I didn't realize we did that. Yeah, I think we should switch to "later overrides earlier". There is no need for "-o" to behave completely differently than all of our other options.

-Peff
Jeff King· Jun 18, 2015, 20:22 UTC · re: Jeff King · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

On Thu, Jun 18, 2015 at 04:13:23PM -0400, Jeff King wrote:
Show 11 quoted lines
> > > You would also need to remove the "oh you gave me -o twice?" check,
> > > and change the semantics to "later -o overrides an earlier one",
> > > wouldn't you?  Otherwise you would never be able to override what
> > > you read from the config, I am afraid.
> > 
> > By the way, I actually think "later -o overrides an earlier one" is
> > a good change by itself, regardless of this new configuration.
> 
> Ah, I didn't realize we did that. Yeah, I think we should switch to
> "later overrides earlier". There is no need for "-o" to behave
> completely differently than all of our other options.

Much worse, though, is that we also have to interact with "--stdout". We currently treat "--stdout -o foo" as an error; you need a separate config_output_directory to continue to handle that (and allow "--stdout" to override the config).

If I were designing from scratch, I would consider making "-o -" output to stdout, and letting it override a previous "-o" (or vice versa). We could still do that (and make "--stdout" an alias for that), but I don't know if it is worth the trouble (it does change the behavior for anybody who wanted a directory called "-", but IMHO it is more likely to save somebody a headache than create one).

-Peff
Junio C Hamano· Jun 18, 2015, 21:46 UTC · re: Jeff King · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> Much worse, though, is that we also have to interact with "--stdout". We
> currently treat "--stdout -o foo" as an error; you need a separate
> config_output_directory to continue to handle that (and allow "--stdout"
> to override the config).
>
> If I were designing from scratch, I would consider making "-o -" output
> to stdout, and letting it override a previous "-o" (or vice versa). We
> could still do that (and make "--stdout" an alias for that), but I don't
> know if it is worth the trouble (it does change the behavior for anybody
> who wanted a directory called "-", but IMHO it is more likely to save
> somebody a headache than create one).

I agree with "later -o should override an earlier one", but I do not necessarily agree with "'-o -' should be --stdout", for a simple reason that "-o foo" is not "--stdout >foo".

Perhaps something like this to replace builtin/ part of Alexander's patch?

 builtin/log.c | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to builtin/log.c +8 −0
diff --git a/builtin/log.c b/builtin/log.c
index e67671e..e022d62 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -682,6 +682,8 @@ enum {
 	COVER_AUTO
 };
 
+static const char *config_output_directory;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -752,6 +754,9 @@ 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(&config_output_directory, var, value);
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die (_("--subject-prefix and -k are mutually exclusive."));
 	rev.preserve_subject = keep_subject;
 
+	if (!output_directory && !use_stdout)
+		output_directory = config_output_directory;
+
 	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
 	if (argc > 1)
 		die (_("unrecognized argument: %s"), argv[1]);
Jeff King· Jun 19, 2015, 04:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > If I were designing from scratch, I would consider making "-o -" output
> > to stdout, and letting it override a previous "-o" (or vice versa). We
> > could still do that (and make "--stdout" an alias for that), but I don't
> > know if it is worth the trouble (it does change the behavior for anybody
> > who wanted a directory called "-", but IMHO it is more likely to save
> > somebody a headache than create one).
> 
> I agree with "later -o should override an earlier one", but I do not
> necessarily agree with "'-o -' should be --stdout", for a simple
> reason that "-o foo" is not "--stdout >foo".

Good point. At any rate, that was all in my "designing from scratch" hypothetical, so it is doubly not worth considering.

Show 10 quoted lines
> Perhaps something like this to replace builtin/ part of Alexander's
> patch?
> [...]
> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die (_("--subject-prefix and -k are mutually exclusive."));
>  	rev.preserve_subject = keep_subject;
>  
> +	if (!output_directory && !use_stdout)
> +		output_directory = config_output_directory;
> +
Yeah, I think that is the sanest way to do it given the constraints.
-Peff
Alexander Kuleshov· Jun 19, 2015, 07:06 UTC · re: Jeff King · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Hello Jeff and Junio,

Thank you for feedback and help. I think also I need to add yet another test which tests case when configuration option is set and -o passed.

I'll make changes and resend the patch.
Thank you.
2015-06-19 10:14 GMT+06:00 Jeff King <peff@peff.net>:
Show 31 quoted lines
> On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:
>
>> > If I were designing from scratch, I would consider making "-o -" output
>> > to stdout, and letting it override a previous "-o" (or vice versa). We
>> > could still do that (and make "--stdout" an alias for that), but I don't
>> > know if it is worth the trouble (it does change the behavior for anybody
>> > who wanted a directory called "-", but IMHO it is more likely to save
>> > somebody a headache than create one).
>>
>> I agree with "later -o should override an earlier one", but I do not
>> necessarily agree with "'-o -' should be --stdout", for a simple
>> reason that "-o foo" is not "--stdout >foo".
>
> Good point. At any rate, that was all in my "designing from scratch"
> hypothetical, so it is doubly not worth considering.
>
>> Perhaps something like this to replace builtin/ part of Alexander's
>> patch?
>> [...]
>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>               die (_("--subject-prefix and -k are mutually exclusive."));
>>       rev.preserve_subject = keep_subject;
>>
>> +     if (!output_directory && !use_stdout)
>> +             output_directory = config_output_directory;
>> +
>
> Yeah, I think that is the sanest way to do it given the constraints.
>
> -Peff
>
Alexander Kuleshov· Jun 19, 2015, 13:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

2015-06-19 3:46 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
Show 15 quoted lines
> I agree with "later -o should override an earlier one", but I do not
> necessarily agree with "'-o -' should be --stdout", for a simple
> reason that "-o foo" is not "--stdout >foo".
>
> Perhaps something like this to replace builtin/ part of Alexander's
> patch?
>
> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>                 die (_("--subject-prefix and -k are mutually exclusive."));
>         rev.preserve_subject = keep_subject;
>
> +       if (!output_directory && !use_stdout)
> +               output_directory = config_output_directory;
> +
>
But there is following condition above:
 if (!use_stdout)
      output_directory = set_outdir(prefix, output_directory);
After which output_directory will be "./" everytime and
Show 5 quoted lines
>
> +       if (!output_directory && !use_stdout)
> +               output_directory = config_output_directory;
> +
>

will not work here. What if we remove if (output_directory) {}.... and update it as:

    if (!use_stdout) {
        if (!config_output_directory && !output_directory)
            output_directory = set_outdir(prefix, output_directory);
        else if (config_output_directory)
            output_directory = config_output_directory;
        if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
            die_errno(_("Could not create directory '%s'"),
                  output_directory);
    }
    else
        setup_pager();
?
Junio C Hamano· Jun 19, 2015, 15:59 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 31 quoted lines
> 2015-06-19 3:46 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
>> I agree with "later -o should override an earlier one", but I do not
>> necessarily agree with "'-o -' should be --stdout", for a simple
>> reason that "-o foo" is not "--stdout >foo".
>>
>> Perhaps something like this to replace builtin/ part of Alexander's
>> patch?
>>
>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>                 die (_("--subject-prefix and -k are mutually exclusive."));
>>         rev.preserve_subject = keep_subject;
>>
>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;
>> +
>>
>
> But there is following condition above:
>
>  if (!use_stdout)
>       output_directory = set_outdir(prefix, output_directory);
>
> After which output_directory will be "./" everytime and
>
>>
>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;
>> +
>>
>
> will not work here.

I thought I made that "if we did not see '-o dir' on the command line, initialize output_directory to what we read from the config" before we make a call to set_outdir().

What I am missing?  

Puzzled... FWIW, IIRC, the patch you are responding to passed the test you added.

Alexander Kuleshov· Jun 19, 2015, 17:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Show 8 quoted lines
> I thought I made that "if we did not see '-o dir' on the command
> line, initialize output_directory to what we read from the config"
> before we make a call to set_outdir().
>
> What I am missing?
>
> Puzzled...  FWIW, IIRC, the patch you are responding to passed the
> test you added.
Ok, Now we have:
if (!use_stdout)
        output_directory = set_outdir(prefix, output_directory);
else
        setup_pager();
and
if (output_directory) {
    // test that we did not pass use_stdout and mkdir than
}

If we didn't pass --stdout and -o the set_outdir will be called and there is

static const char *set_outdir(const char *prefix, const char *output_directory)
{
    //printf("is_absoulte_path %d\n", is_absolute_path(output_directory));
    if (output_directory && is_absolute_path(output_directory))
        return output_directory;
    if (!prefix || !*prefix) {
        if (output_directory)
            return output_directory;
        return "./";
    }
....
}
So it returns "./", output_directory will not be null. After this
>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;
clause will not be executed never. Or I've missed something?
Thank you.
Alexander Kuleshov· Jun 19, 2015, 17:27 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Ah, you mean to put this check before. Just tested it and many tests are broken. Will look on it now

2015-06-19 23:19 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
Show 47 quoted lines
>> I thought I made that "if we did not see '-o dir' on the command
>> line, initialize output_directory to what we read from the config"
>> before we make a call to set_outdir().
>>
>> What I am missing?
>>
>> Puzzled...  FWIW, IIRC, the patch you are responding to passed the
>> test you added.
>
> Ok, Now we have:
>
> if (!use_stdout)
>         output_directory = set_outdir(prefix, output_directory);
> else
>         setup_pager();
>
> and
>
> if (output_directory) {
>     // test that we did not pass use_stdout and mkdir than
> }
>
> If we didn't pass --stdout and -o the set_outdir will be called
> and there is
>
> static const char *set_outdir(const char *prefix, const char *output_directory)
> {
>     //printf("is_absoulte_path %d\n", is_absolute_path(output_directory));
>     if (output_directory && is_absolute_path(output_directory))
>         return output_directory;
>
>     if (!prefix || !*prefix) {
>         if (output_directory)
>             return output_directory;
>         return "./";
>     }
> ....
> }
>
> So it returns "./", output_directory will not be null. After this
>
>>> +       if (!output_directory && !use_stdout)
>>> +               output_directory = config_output_directory;
>
> clause will not be executed never. Or I've missed something?
>
> Thank you.
Alexander Kuleshov· Jun 19, 2015, 17:49 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Sorry for the noise guys, was my fault.

Junio, now all is working and I'm going to send v2. How to send it better in one patch or separate patches for the documentation, tests and etc..?

Thank you.
Junio C Hamano· Jun 19, 2015, 18:14 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov <kuleshovmail@gmail.com> writes:
> Ah, you mean to put this check before.

I am fuzzy what you mean "before" (or "after"); the "how about doing it this way instead?" patch we are discussing is to replace the change you did in your original, so if you apply it you would know what addition goes to where ;-)

Remi Galan Alfonso· Jun 19, 2015, 11:34 UTC · re: Alexander Kuleshov · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 8 quoted lines
> +test_expect_success "format-patch format.outputDirectory option" '
> + git config format.outputDirectory "patches/" &&
> + git format-patch master..side &&
> + cnt=$(ls | wc -l) &&
> + echo $cnt &&
> + test $cnt = 3 &&
> + git config --unset format.outputDirectory
> +'
You should probably do:
> + test_config format.outputDirectory "patches/" &&
instead of:
> + git config format.outputDirectory "patches/" &&
> [...]
> + git config --unset format.outputDirectory

This way there shouldn't be any problem with the tests following yours if your test fails in the middle.

Rémi
Alexander Kuleshov· Jun 19, 2015, 11:34 UTC · re: Remi Galan Alfonso · lore

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Hello,
Yes, thank you for advice.

2015-06-19 17:34 GMT+06:00 Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>:

Show 22 quoted lines
> Alexander Kuleshov <kuleshovmail@gmail.com> writes:
>> +test_expect_success "format-patch format.outputDirectory option" '
>> + git config format.outputDirectory "patches/" &&
>> + git format-patch master..side &&
>> + cnt=$(ls | wc -l) &&
>> + echo $cnt &&
>> + test $cnt = 3 &&
>> + git config --unset format.outputDirectory
>> +'
>
> You should probably do:
>> + test_config format.outputDirectory "patches/" &&
>
> instead of:
>> + git config format.outputDirectory "patches/" &&
>> [...]
>> + git config --unset format.outputDirectory
>
> This way there shouldn't be any problem with the
> tests following yours if your test fails in the middle.
>
> Rémi

← back to recent threads