* [PATCH] format-patch: introduce format.outputDirectory configuration
@ 2015-06-18 11:18 Alexander Kuleshov
2015-06-18 17:13 ` Junio C Hamano
2015-06-19 11:34 ` Remi Galan Alfonso
0 siblings, 2 replies; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-18 11:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Alexander Kuleshov
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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
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
2015-06-19 11:34 ` Remi Galan Alfonso
1 sibling, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2015-06-18 17:13 UTC (permalink / raw)
To: Alexander Kuleshov; +Cc: git
Alexander Kuleshov <kuleshovmail@gmail.com> writes:
> 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?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 17:13 ` Junio C Hamano
@ 2015-06-18 19:57 ` Jeff King
2015-06-18 20:05 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Jeff King @ 2015-06-18 19:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alexander Kuleshov, git
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)
{
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 19:57 ` Jeff King
@ 2015-06-18 20:05 ` Junio C Hamano
2015-06-18 20:06 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2015-06-18 20:05 UTC (permalink / raw)
To: Jeff King; +Cc: Alexander Kuleshov, git
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 20:05 ` Junio C Hamano
@ 2015-06-18 20:06 ` Junio C Hamano
2015-06-18 20:13 ` Jeff King
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2015-06-18 20:06 UTC (permalink / raw)
To: Jeff King; +Cc: Alexander Kuleshov, git
Junio C Hamano <gitster@pobox.com> writes:
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 20:06 ` Junio C Hamano
@ 2015-06-18 20:13 ` Jeff King
2015-06-18 20:22 ` Jeff King
0 siblings, 1 reply; 18+ messages in thread
From: Jeff King @ 2015-06-18 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alexander Kuleshov, git
On Thu, Jun 18, 2015 at 01:06:54PM -0700, Junio C Hamano wrote:
> >> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 20:13 ` Jeff King
@ 2015-06-18 20:22 ` Jeff King
2015-06-18 21:46 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Jeff King @ 2015-06-18 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alexander Kuleshov, git
On Thu, Jun 18, 2015 at 04:13:23PM -0400, Jeff King wrote:
> > > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
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 13:33 ` Alexander Kuleshov
0 siblings, 2 replies; 18+ messages in thread
From: Junio C Hamano @ 2015-06-18 21:46 UTC (permalink / raw)
To: Jeff King; +Cc: Alexander Kuleshov, git
Jeff King <peff@peff.net> writes:
> 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(+)
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]);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
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
1 sibling, 1 reply; 18+ messages in thread
From: Jeff King @ 2015-06-19 4:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alexander Kuleshov, git
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 4:14 ` Jeff King
@ 2015-06-19 7:06 ` Alexander Kuleshov
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 7:06 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org
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>:
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 11:34 ` Remi Galan Alfonso
@ 2015-06-19 11:34 ` Alexander Kuleshov
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 11:34 UTC (permalink / raw)
To: Remi Galan Alfonso; +Cc: Junio C Hamano, git@vger.kernel.org
Hello,
Yes, thank you for advice.
2015-06-19 17:34 GMT+06:00 Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr>:
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 11:18 [PATCH] format-patch: introduce format.outputDirectory configuration Alexander Kuleshov
2015-06-18 17:13 ` Junio C Hamano
@ 2015-06-19 11:34 ` Remi Galan Alfonso
2015-06-19 11:34 ` Alexander Kuleshov
1 sibling, 1 reply; 18+ messages in thread
From: Remi Galan Alfonso @ 2015-06-19 11:34 UTC (permalink / raw)
To: Alexander Kuleshov; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-18 21:46 ` Junio C Hamano
2015-06-19 4:14 ` Jeff King
@ 2015-06-19 13:33 ` Alexander Kuleshov
2015-06-19 15:59 ` Junio C Hamano
1 sibling, 1 reply; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 13:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git@vger.kernel.org
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. 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();
?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 13:33 ` Alexander Kuleshov
@ 2015-06-19 15:59 ` Junio C Hamano
2015-06-19 17:19 ` Alexander Kuleshov
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2015-06-19 15:59 UTC (permalink / raw)
To: Alexander Kuleshov; +Cc: Jeff King, git@vger.kernel.org
Alexander Kuleshov <kuleshovmail@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 15:59 ` Junio C Hamano
@ 2015-06-19 17:19 ` Alexander Kuleshov
2015-06-19 17:27 ` Alexander Kuleshov
0 siblings, 1 reply; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 17:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git@vger.kernel.org
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
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
0 siblings, 2 replies; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git@vger.kernel.org
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>:
>> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 17:27 ` Alexander Kuleshov
@ 2015-06-19 17:49 ` Alexander Kuleshov
2015-06-19 18:14 ` Junio C Hamano
1 sibling, 0 replies; 18+ messages in thread
From: Alexander Kuleshov @ 2015-06-19 17:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] format-patch: introduce format.outputDirectory configuration
2015-06-19 17:27 ` Alexander Kuleshov
2015-06-19 17:49 ` Alexander Kuleshov
@ 2015-06-19 18:14 ` Junio C Hamano
1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2015-06-19 18:14 UTC (permalink / raw)
To: Alexander Kuleshov; +Cc: Jeff King, git@vger.kernel.org
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 ;-)
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-06-19 18:14 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.