{"thread":{"id":"39669","subject":"[PATCH] format-patch: introduce format.outputDirectory configuration","startedAt":"2015-06-18T11:18:00Z","lastAt":"2015-06-19T18:14:34Z","messageCount":18,"participants":["Alexander Kuleshov","Junio C Hamano","Jeff King","Remi Galan Alfonso"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264136","messageId":"1434626280-4610-1-git-send-email-kuleshovmail@gmail.com","threadId":"39669","inReplyTo":null,"subject":"[PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-18T11:18:00Z","receivedAt":"2015-06-18T11:18:00Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"We can pass -o/--output-directory to the format-patch command to\nstore patches not in the working directory. This patch introduces\nformat.outputDirectory configuration option for same purpose.\n\nThe case of usage of this configuration option can be convinience\nto not pass everytime -o/--output-directory if an user has pattern\nto store all patches in the /patches directory for example.\n\nThe format.outputDirectory has lower priority than command line\noption, so if user will set format.outputDirectory and pass the\ncommand line option, a result will be stored in a directory that\npassed to command line option.\n\nSigned-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n---\n Documentation/config.txt |  4 ++++\n builtin/log.c            | 14 ++++++++++++--\n t/t4014-format-patch.sh  |  9 +++++++++\n 3 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fd2036c..8f6f7ed 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1247,6 +1247,10 @@ format.coverLetter::\n \tformat-patch is invoked, but in addition can be set to \"auto\", to\n \tgenerate a cover-letter only when there's more than one patch.\n \n+format.outputDirectory::\n+\tSet a custom directory to store the resulting files instead of the\n+\tcurrent working directory.\n+\n filter.<driver>.clean::\n \tThe command which is used to convert the content of a worktree\n \tfile to a blob upon checkin.  See linkgit:gitattributes[5] for\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dfb351e..22c1e46 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -687,6 +687,8 @@ enum {\n \tCOVER_AUTO\n };\n \n+static const char *config_output_directory = NULL;\n+\n static int git_format_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"format.headers\")) {\n@@ -757,6 +759,9 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\tconfig_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.outputdirectory\")) {\n+\t\treturn git_config_string(&config_output_directory, var, value);\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -1006,7 +1011,8 @@ static const char *clean_message_id(const char *msg_id)\n \treturn xmemdupz(a, z - a);\n }\n \n-static const char *set_outdir(const char *prefix, const char *output_directory)\n+static const char *set_outdir(const char *prefix, const char *output_directory,\n+\t\t\t      const char *config_output_directory)\n {\n \tif (output_directory && is_absolute_path(output_directory))\n \t\treturn output_directory;\n@@ -1014,6 +1020,9 @@ static const char *set_outdir(const char *prefix, const char *output_directory)\n \tif (!prefix || !*prefix) {\n \t\tif (output_directory)\n \t\t\treturn output_directory;\n+\n+\t\tif (config_output_directory)\n+\t\t\treturn config_output_directory;\n \t\t/* The user did not explicitly ask for \"./\" */\n \t\toutdir_offset = 2;\n \t\treturn \"./\";\n@@ -1368,7 +1377,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tinit_display_notes(&rev.notes_opt);\n \n \tif (!use_stdout)\n-\t\toutput_directory = set_outdir(prefix, output_directory);\n+\t\toutput_directory = set_outdir(prefix, output_directory,\n+\t\t\t\t\t      config_output_directory);\n \telse\n \t\tsetup_pager();\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex c39e500..a4b18b5 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -40,6 +40,15 @@ test_expect_success setup '\n \n '\n \n+test_expect_success \"format-patch format.outputDirectory option\" '\n+\tgit config format.outputDirectory \"patches/\" &&\n+\tgit format-patch master..side &&\n+\tcnt=$(ls | wc -l) &&\n+\techo $cnt &&\n+\ttest $cnt = 3 &&\n+\tgit config --unset format.outputDirectory\n+'\n+\n test_expect_success \"format-patch --ignore-if-in-upstream\" '\n \n \tgit format-patch --stdout master..side >patch0 &&\n-- \n2.4.0.383.gded6615.dirty\n"},{"id":"264201","messageId":"xmqq616ley7y.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"1434626280-4610-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T17:13:37Z","receivedAt":"2015-06-18T17:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index fd2036c..8f6f7ed 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1247,6 +1247,10 @@ format.coverLetter::\n>  \tformat-patch is invoked, but in addition can be set to \"auto\", to\n>  \tgenerate a cover-letter only when there's more than one patch.\n>  \n> +format.outputDirectory::\n> +\tSet a custom directory to store the resulting files instead of the\n> +\tcurrent working directory.\n> +\n\nAfter you set this configuration variable, how would you override it\nand get the default behaviour back from the command line for one\ntime invocation?  \"-o ./\"?  That needs to be documented somewhere.\n\nDocumentation/format-patch.txt must have description on -o; that\nparagraph needs to mention this new configuration variable, and it\nwould be a good place to document the \"-o ./\" workaround.\n\n> -static const char *set_outdir(const char *prefix, const char *output_directory)\n> +static const char *set_outdir(const char *prefix, const char *output_directory,\n> +\t\t\t      const char *config_output_directory)\n\nThis change looks ugly and unnecessary.  All the machinery after and\nincluding the point set_outdir() is called, including reopen_stdout(),\nwork on output_directory variable and only that variable.\n\nWouldn't it work equally well to have\n\n\tif (!output_directory)\n        \toutput_directory = config_output_directory;\n\nbefore a call to set_outdir() is made but after the configuration is\nread (namely, soon after parse_options() returns), without making\nany change to this function?\n"},{"id":"264211","messageId":"20150618195751.GA14550@peff.net","threadId":"39669","inReplyTo":"xmqq616ley7y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-18T19:57:51Z","receivedAt":"2015-06-18T19:57:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 18, 2015 at 10:13:37AM -0700, Junio C Hamano wrote:\n\n> > -static const char *set_outdir(const char *prefix, const char *output_directory)\n> > +static const char *set_outdir(const char *prefix, const char *output_directory,\n> > +\t\t\t      const char *config_output_directory)\n> \n> This change looks ugly and unnecessary.  All the machinery after and\n> including the point set_outdir() is called, including reopen_stdout(),\n> work on output_directory variable and only that variable.\n> \n> Wouldn't it work equally well to have\n> \n> \tif (!output_directory)\n>         \toutput_directory = config_output_directory;\n> \n> before a call to set_outdir() is made but after the configuration is\n> read (namely, soon after parse_options() returns), without making\n> any change to this function?\n\nDon't we load the config before parsing options here? In that case, we\ncan use our usual strategy to just set output_directory (which is\nalready a static global) from the config callback, and everything Just\nWorks.\n\nWe do have to bump the definition of output_directory up above the\nconfig callback, like so (while we are here, we might also want to\ndrop the unnecessary static initializers, which violate our style guide):\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e67671e..77c06f7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -37,6 +37,10 @@ static int use_mailmap_config;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n \n+static FILE *realstdout = NULL;\n+static const char *output_directory = NULL;\n+static int outdir_offset;\n+\n static const char * const builtin_log_usage[] = {\n \tN_(\"git log [<options>] [<revision-range>] [[--] <path>...]\"),\n \tN_(\"git show [<options>] <object>...\"),\n@@ -752,14 +756,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\tconfig_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.outputdirectory\"))\n+\t\treturn git_config_string(&output_directory, var, value);\n \n \treturn git_log_config(var, value, cb);\n }\n \n-static FILE *realstdout = NULL;\n-static const char *output_directory = NULL;\n-static int outdir_offset;\n-\n static int reopen_stdout(struct commit *commit, const char *subject,\n \t\t\t struct rev_info *rev, int quiet)\n {\n"},{"id":"264213","messageId":"xmqqoakceq8s.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"20150618195751.GA14550@peff.net","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T20:05:55Z","receivedAt":"2015-06-18T20:05:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> This change looks ugly and unnecessary.  All the machinery after and\n>> including the point set_outdir() is called, including reopen_stdout(),\n>> work on output_directory variable and only that variable.\n>> \n>> Wouldn't it work equally well to have\n>> \n>> \tif (!output_directory)\n>>         \toutput_directory = config_output_directory;\n>> \n>> before a call to set_outdir() is made but after the configuration is\n>> read (namely, soon after parse_options() returns), without making\n>> any change to this function?\n>\n> Don't we load the config before parsing options here? In that case, we\n> can use our usual strategy to just set output_directory (which is\n> already a static global) from the config callback, and everything Just\n> Works.\n>\n> We do have to bump the definition of output_directory up above the\n> config callback, like so (while we are here, we might also want to\n> drop the unnecessary static initializers, which violate our style guide):\n\nYou would also need to remove the \"oh you gave me -o twice?\" check,\nand change the semantics to \"later -o overrides an earlier one\",\nwouldn't you?  Otherwise you would never be able to override what\nyou read from the config, I am afraid.\n"},{"id":"264214","messageId":"xmqqk2v0eq75.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"xmqqoakceq8s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T20:06:54Z","receivedAt":"2015-06-18T20:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>>> This change looks ugly and unnecessary.  All the machinery after and\n>>> including the point set_outdir() is called, including reopen_stdout(),\n>>> work on output_directory variable and only that variable.\n>>> \n>>> Wouldn't it work equally well to have\n>>> \n>>> \tif (!output_directory)\n>>>         \toutput_directory = config_output_directory;\n>>> \n>>> before a call to set_outdir() is made but after the configuration is\n>>> read (namely, soon after parse_options() returns), without making\n>>> any change to this function?\n>>\n>> Don't we load the config before parsing options here? In that case, we\n>> can use our usual strategy to just set output_directory (which is\n>> already a static global) from the config callback, and everything Just\n>> Works.\n>>\n>> We do have to bump the definition of output_directory up above the\n>> config callback, like so (while we are here, we might also want to\n>> drop the unnecessary static initializers, which violate our style guide):\n>\n> You would also need to remove the \"oh you gave me -o twice?\" check,\n> and change the semantics to \"later -o overrides an earlier one\",\n> wouldn't you?  Otherwise you would never be able to override what\n> you read from the config, I am afraid.\n\nBy the way, I actually think \"later -o overrides an earlier one\" is\na good change by itself, regardless of this new configuration.\n"},{"id":"264236","messageId":"20150618201323.GB14550@peff.net","threadId":"39669","inReplyTo":"xmqqk2v0eq75.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-18T20:13:24Z","receivedAt":"2015-06-18T20:13:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 18, 2015 at 01:06:54PM -0700, Junio C Hamano wrote:\n\n> >> Don't we load the config before parsing options here? In that case, we\n> >> can use our usual strategy to just set output_directory (which is\n> >> already a static global) from the config callback, and everything Just\n> >> Works.\n> >>\n> >> We do have to bump the definition of output_directory up above the\n> >> config callback, like so (while we are here, we might also want to\n> >> drop the unnecessary static initializers, which violate our style guide):\n> >\n> > You would also need to remove the \"oh you gave me -o twice?\" check,\n> > and change the semantics to \"later -o overrides an earlier one\",\n> > wouldn't you?  Otherwise you would never be able to override what\n> > you read from the config, I am afraid.\n> \n> By the way, I actually think \"later -o overrides an earlier one\" is\n> a good change by itself, regardless of this new configuration.\n\nAh, I didn't realize we did that. Yeah, I think we should switch to\n\"later overrides earlier\". There is no need for \"-o\" to behave\ncompletely differently than all of our other options.\n\n-Peff\n"},{"id":"264239","messageId":"20150618202205.GA16517@peff.net","threadId":"39669","inReplyTo":"20150618201323.GB14550@peff.net","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-18T20:22:05Z","receivedAt":"2015-06-18T20:22:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 18, 2015 at 04:13:23PM -0400, Jeff King wrote:\n\n> > > You would also need to remove the \"oh you gave me -o twice?\" check,\n> > > and change the semantics to \"later -o overrides an earlier one\",\n> > > wouldn't you?  Otherwise you would never be able to override what\n> > > you read from the config, I am afraid.\n> > \n> > By the way, I actually think \"later -o overrides an earlier one\" is\n> > a good change by itself, regardless of this new configuration.\n> \n> Ah, I didn't realize we did that. Yeah, I think we should switch to\n> \"later overrides earlier\". There is no need for \"-o\" to behave\n> completely differently than all of our other options.\n\nMuch worse, though, is that we also have to interact with \"--stdout\". We\ncurrently treat \"--stdout -o foo\" as an error; you need a separate\nconfig_output_directory to continue to handle that (and allow \"--stdout\"\nto override the config).\n\nIf I were designing from scratch, I would consider making \"-o -\" output\nto stdout, and letting it override a previous \"-o\" (or vice versa). We\ncould still do that (and make \"--stdout\" an alias for that), but I don't\nknow if it is worth the trouble (it does change the behavior for anybody\nwho wanted a directory called \"-\", but IMHO it is more likely to save\nsomebody a headache than create one).\n\n-Peff\n"},{"id":"264256","messageId":"xmqqd20sd70j.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"20150618202205.GA16517@peff.net","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T21:46:36Z","receivedAt":"2015-06-18T21:46:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Much worse, though, is that we also have to interact with \"--stdout\". We\n> currently treat \"--stdout -o foo\" as an error; you need a separate\n> config_output_directory to continue to handle that (and allow \"--stdout\"\n> to override the config).\n>\n> If I were designing from scratch, I would consider making \"-o -\" output\n> to stdout, and letting it override a previous \"-o\" (or vice versa). We\n> could still do that (and make \"--stdout\" an alias for that), but I don't\n> know if it is worth the trouble (it does change the behavior for anybody\n> who wanted a directory called \"-\", but IMHO it is more likely to save\n> somebody a headache than create one).\n\nI agree with \"later -o should override an earlier one\", but I do not\nnecessarily agree with \"'-o -' should be --stdout\", for a simple\nreason that \"-o foo\" is not \"--stdout >foo\".\n\nPerhaps something like this to replace builtin/ part of Alexander's\npatch?\n\n builtin/log.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e67671e..e022d62 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -682,6 +682,8 @@ enum {\n \tCOVER_AUTO\n };\n \n+static const char *config_output_directory;\n+\n static int git_format_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"format.headers\")) {\n@@ -752,6 +754,9 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\tconfig_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.outputdirectory\")) {\n+\t\treturn git_config_string(&config_output_directory, var, value);\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tdie (_(\"--subject-prefix and -k are mutually exclusive.\"));\n \trev.preserve_subject = keep_subject;\n \n+\tif (!output_directory && !use_stdout)\n+\t\toutput_directory = config_output_directory;\n+\n \targc = setup_revisions(argc, argv, &rev, &s_r_opt);\n \tif (argc > 1)\n \t\tdie (_(\"unrecognized argument: %s\"), argv[1]);\n"},{"id":"264272","messageId":"20150619041437.GA26001@peff.net","threadId":"39669","inReplyTo":"xmqqd20sd70j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-19T04:14:38Z","receivedAt":"2015-06-19T04:14:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:\n\n> > If I were designing from scratch, I would consider making \"-o -\" output\n> > to stdout, and letting it override a previous \"-o\" (or vice versa). We\n> > could still do that (and make \"--stdout\" an alias for that), but I don't\n> > know if it is worth the trouble (it does change the behavior for anybody\n> > who wanted a directory called \"-\", but IMHO it is more likely to save\n> > somebody a headache than create one).\n> \n> I agree with \"later -o should override an earlier one\", but I do not\n> necessarily agree with \"'-o -' should be --stdout\", for a simple\n> reason that \"-o foo\" is not \"--stdout >foo\".\n\nGood point. At any rate, that was all in my \"designing from scratch\"\nhypothetical, so it is doubly not worth considering.\n\n> Perhaps something like this to replace builtin/ part of Alexander's\n> patch?\n> [...]\n> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\tdie (_(\"--subject-prefix and -k are mutually exclusive.\"));\n>  \trev.preserve_subject = keep_subject;\n>  \n> +\tif (!output_directory && !use_stdout)\n> +\t\toutput_directory = config_output_directory;\n> +\n\nYeah, I think that is the sanest way to do it given the constraints.\n\n-Peff\n"},{"id":"264277","messageId":"CANCZXo5qu=+u4BXWeOgX2r6T8jn-ysp9XfLhDc7Ca=UcvrzR4w@mail.gmail.com","threadId":"39669","inReplyTo":"20150619041437.GA26001@peff.net","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T07:06:00Z","receivedAt":"2015-06-19T07:06:00Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Jeff and Junio,\n\nThank you for feedback and help. I think also I need to add yet another test\nwhich tests case when configuration option is set and -o passed.\n\nI'll make changes and resend the patch.\n\nThank you.\n\n\n2015-06-19 10:14 GMT+06:00 Jeff King <peff@peff.net>:\n> On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:\n>\n>> > If I were designing from scratch, I would consider making \"-o -\" output\n>> > to stdout, and letting it override a previous \"-o\" (or vice versa). We\n>> > could still do that (and make \"--stdout\" an alias for that), but I don't\n>> > know if it is worth the trouble (it does change the behavior for anybody\n>> > who wanted a directory called \"-\", but IMHO it is more likely to save\n>> > somebody a headache than create one).\n>>\n>> I agree with \"later -o should override an earlier one\", but I do not\n>> necessarily agree with \"'-o -' should be --stdout\", for a simple\n>> reason that \"-o foo\" is not \"--stdout >foo\".\n>\n> Good point. At any rate, that was all in my \"designing from scratch\"\n> hypothetical, so it is doubly not worth considering.\n>\n>> Perhaps something like this to replace builtin/ part of Alexander's\n>> patch?\n>> [...]\n>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>>               die (_(\"--subject-prefix and -k are mutually exclusive.\"));\n>>       rev.preserve_subject = keep_subject;\n>>\n>> +     if (!output_directory && !use_stdout)\n>> +             output_directory = config_output_directory;\n>> +\n>\n> Yeah, I think that is the sanest way to do it given the constraints.\n>\n> -Peff\n>\n"},{"id":"264304","messageId":"CANCZXo7j=5zcjhxXAeEKagRmUVTNVyaDTzyt1LL_-uufGARCKA@mail.gmail.com","threadId":"39669","inReplyTo":"1537731273.629800.1434713654223.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T11:34:09Z","receivedAt":"2015-06-19T11:34:09Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello,\n\nYes, thank you for advice.\n\n2015-06-19 17:34 GMT+06:00 Remi Galan Alfonso\n<remi.galan-alfonso@ensimag.grenoble-inp.fr>:\n> Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n>> +test_expect_success \"format-patch format.outputDirectory option\" '\n>> + git config format.outputDirectory \"patches/\" &&\n>> + git format-patch master..side &&\n>> + cnt=$(ls | wc -l) &&\n>> + echo $cnt &&\n>> + test $cnt = 3 &&\n>> + git config --unset format.outputDirectory\n>> +'\n>\n> You should probably do:\n>> + test_config format.outputDirectory \"patches/\" &&\n>\n> instead of:\n>> + git config format.outputDirectory \"patches/\" &&\n>> [...]\n>> + git config --unset format.outputDirectory\n>\n> This way there shouldn't be any problem with the\n> tests following yours if your test fails in the middle.\n>\n> Rémi\n"},{"id":"264303","messageId":"1537731273.629800.1434713654223.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39669","inReplyTo":"1434626280-4610-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-19T11:34:14Z","receivedAt":"2015-06-19T11:34:14Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n> +test_expect_success \"format-patch format.outputDirectory option\" '\n> + git config format.outputDirectory \"patches/\" &&\n> + git format-patch master..side &&\n> + cnt=$(ls | wc -l) &&\n> + echo $cnt &&\n> + test $cnt = 3 &&\n> + git config --unset format.outputDirectory\n> +'\n\nYou should probably do:\n> + test_config format.outputDirectory \"patches/\" &&\n\ninstead of:\n> + git config format.outputDirectory \"patches/\" &&\n> [...]\n> + git config --unset format.outputDirectory\n\nThis way there shouldn't be any problem with the \ntests following yours if your test fails in the middle.\n\nRémi\n"},{"id":"264312","messageId":"CANCZXo72BscpXKGAtVPt_1QuffcOpTz6nGB+__q0JLisuTaKsQ@mail.gmail.com","threadId":"39669","inReplyTo":"xmqqd20sd70j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T13:33:01Z","receivedAt":"2015-06-19T13:33:01Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"2015-06-19 3:46 GMT+06:00 Junio C Hamano <gitster@pobox.com>:\n> I agree with \"later -o should override an earlier one\", but I do not\n> necessarily agree with \"'-o -' should be --stdout\", for a simple\n> reason that \"-o foo\" is not \"--stdout >foo\".\n>\n> Perhaps something like this to replace builtin/ part of Alexander's\n> patch?\n>\n> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>                 die (_(\"--subject-prefix and -k are mutually exclusive.\"));\n>         rev.preserve_subject = keep_subject;\n>\n> +       if (!output_directory && !use_stdout)\n> +               output_directory = config_output_directory;\n> +\n>\n\nBut there is following condition above:\n\n if (!use_stdout)\n      output_directory = set_outdir(prefix, output_directory);\n\nAfter which output_directory will be \"./\" everytime and\n\n>\n> +       if (!output_directory && !use_stdout)\n> +               output_directory = config_output_directory;\n> +\n>\n\nwill not work here. What if we remove if (output_directory) {}....\nand update it as:\n\n    if (!use_stdout) {\n        if (!config_output_directory && !output_directory)\n            output_directory = set_outdir(prefix, output_directory);\n        else if (config_output_directory)\n            output_directory = config_output_directory;\n\n        if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)\n            die_errno(_(\"Could not create directory '%s'\"),\n                  output_directory);\n    }\n    else\n        setup_pager();\n\n?\n"},{"id":"264336","messageId":"xmqq616jbse8.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"CANCZXo72BscpXKGAtVPt_1QuffcOpTz6nGB+__q0JLisuTaKsQ@mail.gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T15:59:59Z","receivedAt":"2015-06-19T15:59:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> 2015-06-19 3:46 GMT+06:00 Junio C Hamano <gitster@pobox.com>:\n>> I agree with \"later -o should override an earlier one\", but I do not\n>> necessarily agree with \"'-o -' should be --stdout\", for a simple\n>> reason that \"-o foo\" is not \"--stdout >foo\".\n>>\n>> Perhaps something like this to replace builtin/ part of Alexander's\n>> patch?\n>>\n>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>>                 die (_(\"--subject-prefix and -k are mutually exclusive.\"));\n>>         rev.preserve_subject = keep_subject;\n>>\n>> +       if (!output_directory && !use_stdout)\n>> +               output_directory = config_output_directory;\n>> +\n>>\n>\n> But there is following condition above:\n>\n>  if (!use_stdout)\n>       output_directory = set_outdir(prefix, output_directory);\n>\n> After which output_directory will be \"./\" everytime and\n>\n>>\n>> +       if (!output_directory && !use_stdout)\n>> +               output_directory = config_output_directory;\n>> +\n>>\n>\n> will not work here.\n\nI thought I made that \"if we did not see '-o dir' on the command\nline, initialize output_directory to what we read from the config\"\nbefore we make a call to set_outdir().\n\nWhat I am missing?  \n\nPuzzled...  FWIW, IIRC, the patch you are responding to passed the\ntest you added.\n"},{"id":"264341","messageId":"CANCZXo5Nyt+JePQP=kvFsjTaV=xKXduoBqAwp5E0CrEf13QK7g@mail.gmail.com","threadId":"39669","inReplyTo":"xmqq616jbse8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T17:19:26Z","receivedAt":"2015-06-19T17:19:26Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"> I thought I made that \"if we did not see '-o dir' on the command\n> line, initialize output_directory to what we read from the config\"\n> before we make a call to set_outdir().\n>\n> What I am missing?\n>\n> Puzzled...  FWIW, IIRC, the patch you are responding to passed the\n> test you added.\n\nOk, Now we have:\n\nif (!use_stdout)\n        output_directory = set_outdir(prefix, output_directory);\nelse\n        setup_pager();\n\nand\n\nif (output_directory) {\n    // test that we did not pass use_stdout and mkdir than\n}\n\nIf we didn't pass --stdout and -o the set_outdir will be called\nand there is\n\nstatic const char *set_outdir(const char *prefix, const char *output_directory)\n{\n    //printf(\"is_absoulte_path %d\\n\", is_absolute_path(output_directory));\n    if (output_directory && is_absolute_path(output_directory))\n        return output_directory;\n\n    if (!prefix || !*prefix) {\n        if (output_directory)\n            return output_directory;\n        return \"./\";\n    }\n....\n}\n\nSo it returns \"./\", output_directory will not be null. After this\n\n>> +       if (!output_directory && !use_stdout)\n>> +               output_directory = config_output_directory;\n\nclause will not be executed never. Or I've missed something?\n\nThank you.\n"},{"id":"264343","messageId":"CANCZXo7Lyo_Sb=bxF9EgZHV35JfrQZ-CFsM9T4yUjkDBndcp8A@mail.gmail.com","threadId":"39669","inReplyTo":"CANCZXo5Nyt+JePQP=kvFsjTaV=xKXduoBqAwp5E0CrEf13QK7g@mail.gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T17:27:20Z","receivedAt":"2015-06-19T17:27:20Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Ah, you mean to put this check before. Just tested it and\nmany tests are broken. Will look on it now\n\n2015-06-19 23:19 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:\n>> I thought I made that \"if we did not see '-o dir' on the command\n>> line, initialize output_directory to what we read from the config\"\n>> before we make a call to set_outdir().\n>>\n>> What I am missing?\n>>\n>> Puzzled...  FWIW, IIRC, the patch you are responding to passed the\n>> test you added.\n>\n> Ok, Now we have:\n>\n> if (!use_stdout)\n>         output_directory = set_outdir(prefix, output_directory);\n> else\n>         setup_pager();\n>\n> and\n>\n> if (output_directory) {\n>     // test that we did not pass use_stdout and mkdir than\n> }\n>\n> If we didn't pass --stdout and -o the set_outdir will be called\n> and there is\n>\n> static const char *set_outdir(const char *prefix, const char *output_directory)\n> {\n>     //printf(\"is_absoulte_path %d\\n\", is_absolute_path(output_directory));\n>     if (output_directory && is_absolute_path(output_directory))\n>         return output_directory;\n>\n>     if (!prefix || !*prefix) {\n>         if (output_directory)\n>             return output_directory;\n>         return \"./\";\n>     }\n> ....\n> }\n>\n> So it returns \"./\", output_directory will not be null. After this\n>\n>>> +       if (!output_directory && !use_stdout)\n>>> +               output_directory = config_output_directory;\n>\n> clause will not be executed never. Or I've missed something?\n>\n> Thank you.\n"},{"id":"264346","messageId":"CANCZXo4b63SHj9XP0VP+O404ittZGWjJAnzQy36Oqy1EoSOUHw@mail.gmail.com","threadId":"39669","inReplyTo":"CANCZXo7Lyo_Sb=bxF9EgZHV35JfrQZ-CFsM9T4yUjkDBndcp8A@mail.gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-06-19T17:49:31Z","receivedAt":"2015-06-19T17:49:31Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Sorry for the noise guys, was my fault.\n\nJunio, now all is working and I'm going to send v2.\nHow to send it better in one patch or separate patches\nfor the documentation, tests and etc..?\n\nThank you.\n"},{"id":"264349","messageId":"xmqqwpyz8t11.fsf@gitster.dls.corp.google.com","threadId":"39669","inReplyTo":"CANCZXo7Lyo_Sb=bxF9EgZHV35JfrQZ-CFsM9T4yUjkDBndcp8A@mail.gmail.com","subject":"Re: [PATCH] format-patch: introduce format.outputDirectory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T18:14:34Z","receivedAt":"2015-06-19T18:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> Ah, you mean to put this check before.\n\nI am fuzzy what you mean \"before\" (or \"after\"); the \"how about doing\nit this way instead?\" patch we are discussing is to replace the\nchange you did in your original, so if you apply it you would know\nwhat addition goes to where ;-)\n"}]}