{"thread":{"id":"52515","subject":"[PATCH] format-patch: make format.outputDirectory relative to worktree","startedAt":"2019-12-24T12:38:02Z","lastAt":"2019-12-25T02:26:26Z","messageCount":4,"participants":["Denton Liu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"388866","messageId":"9422e4225143b2b908bd1fed9a510b9333bf34a2.1577191015.git.liu.denton@gmail.com","threadId":"52515","inReplyTo":null,"subject":"[PATCH] format-patch: make format.outputDirectory relative to worktree","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-24T12:37:50Z","receivedAt":"2019-12-24T12:38:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, when format-patch outputs patches to the path specified by\nthe `format.outputDirectory` configuration variable, it is relative to\nthe current working directory. However, it would make more sense for the\npath to be relative to the worktree if it exists so that patches will be\nplaced in one location even if a user were to change directories.\n\nRewrite the output directory logic for format-patch so that it will be\nrelative to the worktree of the directory. An escape hatch is provided\nfor if the previous behaviour is desired by prepending \"./\" to the\nvariable.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Documentation/config/format.txt |  3 +++\n builtin/log.c                   | 20 ++++++++++++-----\n t/t4014-format-patch.sh         | 39 +++++++++++++++++++++++++++++++++\n 3 files changed, 56 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\nindex 513fcd88d5..fc9d110d88 100644\n--- a/Documentation/config/format.txt\n+++ b/Documentation/config/format.txt\n@@ -88,6 +88,9 @@ format.coverLetter::\n format.outputDirectory::\n \tSet a custom directory to store the resulting files instead of the\n \tcurrent working directory. All directory components will be created.\n+\tThe path specified will be relative to the worktree of the repository\n+\tunless the path begins with \"./\" or there is no worktree in which case\n+\tthe path will be relative to current working directory.\n \n format.useAutoBase::\n \tA boolean value which lets you enable the `--base=auto` option of\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8c664067ca..fe072c7309 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -885,7 +885,7 @@ static int git_format_config(const char *var, const char *value, void *cb)\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\treturn git_config_pathname(&config_output_directory, var, value);\n \tif (!strcmp(var, \"format.useautobase\")) {\n \t\tbase_auto = git_config_bool(var, value);\n \t\treturn 0;\n@@ -1841,13 +1841,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tinit_display_notes(&rev.notes_opt);\n \n-\tif (!output_directory && !use_stdout)\n-\t\toutput_directory = config_output_directory;\n+\tif (!use_stdout) {\n+\t\tconst char *outdir_prefix = NULL;\n+\t\tconst char *outdir = config_output_directory;\n \n-\tif (!use_stdout)\n-\t\toutput_directory = set_outdir(prefix, output_directory);\n-\telse\n+\t\tif (output_directory)\n+\t\t\toutdir = output_directory;\n+\n+\t\tif (output_directory ||\n+\t\t    (config_output_directory && starts_with(config_output_directory, \"./\")))\n+\t\t\toutdir_prefix = prefix;\n+\n+\t\toutput_directory = set_outdir(outdir_prefix, outdir);\n+\t} else {\n \t\tsetup_pager();\n+\t}\n \n \tif (output_directory) {\n \t\tint saved;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex a5b6302a1c..cc13cc4699 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1813,6 +1813,45 @@ test_expect_success 'format-patch format.outputDirectory option' '\n \ttest_line_count = $count list\n '\n \n+test_expect_success 'format-patch format.outputDirectory option relative to git dir' '\n+\ttest_config format.outputDirectory patches &&\n+\trm -fr patches &&\n+\tmkdir -p sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit format-patch master..side\n+\t) &&\n+\tcount=$(git rev-list --count master..side) &&\n+\tls patches >list &&\n+\ttest_line_count = $count list\n+'\n+\n+test_expect_success 'format-patch format.outputDirectory option with relative path' '\n+\ttest_config format.outputDirectory ./patches &&\n+\tmkdir -p sub &&\n+\t(\n+\t\tcd sub &&\n+\t\trm -fr patches &&\n+\t\tgit format-patch master..side &&\n+\t\tcount=$(git rev-list --count master..side) &&\n+\t\tls patches >list &&\n+\t\ttest_line_count = $count list\n+\t)\n+'\n+\n+test_expect_success 'format-patch format.outputDirectory option absolute path' '\n+\ttest_config format.outputDirectory \"$PWD/patches\" &&\n+\trm -fr patches &&\n+\tmkdir -p sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit format-patch master..side\n+\t) &&\n+\tcount=$(git rev-list --count master..side) &&\n+\tls patches >list &&\n+\ttest_line_count = $count list\n+'\n+\n test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \ttest_config format.outputDirectory patches &&\n \trm -fr patches patchset &&\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388873","messageId":"xmqq36d95xwh.fsf@gitster-ct.c.googlers.com","threadId":"52515","inReplyTo":"9422e4225143b2b908bd1fed9a510b9333bf34a2.1577191015.git.liu.denton@gmail.com","subject":"Re: [PATCH] format-patch: make format.outputDirectory relative to worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-24T19:25:18Z","receivedAt":"2019-12-24T19:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Currently, when format-patch outputs patches to the path specified by\n> the `format.outputDirectory` configuration variable, it is relative to\n> the current working directory. However, it would make more sense for the\n> path to be relative to the worktree if it exists so that patches will be\n> placed in one location even if a user were to change directories.\n\nWould it make more sense?  If a user is deep in the hierarchy, after\nrunning\n\n\t$ git format-patch master..\n\nthe next thing the user would want to do is to go to the directory\nand give them final review and edit the patches before sending out.\nThat would involve going up some levels depending on where s/he is,\nin other words, the location the patches are found would be different\nand this change makes it a lot more cumbersome to go there with a\nrelative path.  The only people who benefit from the change is those\nwho have their working trees at very shallow hierarchy so that it is\nnot cumbersome for them to give the full path to the top of them.\n\nSo I think this cuts both ways, and \"it would make more sense\" is\noverselling.  Besides, those who can easily give the full path to\nthe top of the working tree (because they are short) can spell that\nshort path to outputDirectory configuration variable just once and\nbe done with it, no?\n\n> Rewrite the output directory logic for format-patch so that it will be\n> relative to the worktree of the directory. An escape hatch is provided\n> for if the previous behaviour is desired by prepending \"./\" to the\n> variable.\n\nAnything that forces existing users to change their existing\nsettings is not an escape hatch.  It merely is a regression with a\npossible workaround.\n\nSo I dunno.  I probably would have accepted a patch that *adds*\noutputDirectory configuration that behaves the way you are proposing\nin this patch if we did not have the variable yet in the system\n(i.e. three or four years ago), but I am not sure if it is a good\nidea to change it after these years.\n"},{"id":"388891","messageId":"20191224220544.GA1917@generichostname","threadId":"52515","inReplyTo":"xmqq36d95xwh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] format-patch: make format.outputDirectory relative to worktree","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-24T22:05:44Z","receivedAt":"2019-12-24T22:05:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Tue, Dec 24, 2019 at 11:25:18AM -0800, Junio C Hamano wrote:\n> Denton Liu <liu.denton@gmail.com> writes:\n> > Rewrite the output directory logic for format-patch so that it will be\n> > relative to the worktree of the directory. An escape hatch is provided\n> > for if the previous behaviour is desired by prepending \"./\" to the\n> > variable.\n> \n> Anything that forces existing users to change their existing\n> settings is not an escape hatch.  It merely is a regression with a\n> possible workaround.\n> \n> So I dunno.  I probably would have accepted a patch that *adds*\n> outputDirectory configuration that behaves the way you are proposing\n> in this patch if we did not have the variable yet in the system\n> (i.e. three or four years ago), but I am not sure if it is a good\n> idea to change it after these years.\n\nPerhaps instead of switching the default behaviour so that it's always\nrelative to the top of the worktree, we can add some sort of prefix to\nindicate that we want this behaviour. Maybe `worktree:path/from/top` or\n`^path/from/top` or something like this?\n\nThanks,\n\nDenton\n"},{"id":"388892","messageId":"xmqqtv5p3zuc.fsf@gitster-ct.c.googlers.com","threadId":"52515","inReplyTo":"20191224220544.GA1917@generichostname","subject":"Re: [PATCH] format-patch: make format.outputDirectory relative to worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-25T02:26:19Z","receivedAt":"2019-12-25T02:26:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> ... Maybe `worktree:path/from/top` or\n> `^path/from/top` or something like this?\n\nI recall seeing a similar idea with a bit better syntax discussed\nwhen we talked about path-type values in the configuration system,\nwhere we use \"~WORD~/\" prefix to expand to Git specific special\nplaces in the filesystem, just like \"~USER/\" prefix expands to the\nhome directory of the user.  E.g. \"~gitdir~/config\" may expand to\npoint at the per-repository configuration file in the current\nrepository, and \"~worktree~/Makefile\" may be the top-level Makefile\nchecked out in the current worktree.  This depends on the fact that\nnobody sane has username ending with tilde (and if there is such a\nuser, tough luck for that user).  I'm tempted to credit Dscho for\nthe idea but it could have been by J6t.\n\nDoes anybody have a pointer to the discussion thread?\n"}]}