{"thread":{"id":"34139","subject":"[PATCH] format-patch: remove existing output-directory","startedAt":"2013-06-14T12:43:33Z","lastAt":"2013-06-14T13:58:34Z","messageCount":12,"participants":["Ramkumar Ramachandra","John Keeping","Fredrik Gustafsson","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"220812","messageId":"1371213813-7925-1-git-send-email-artagnon@gmail.com","threadId":"34139","inReplyTo":null,"subject":"[PATCH] format-patch: remove existing output-directory","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-14T12:43:33Z","receivedAt":"2013-06-14T12:43:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The following command\n\n  $ git format-patch -o outgoing master\n\ndoes not ensure that the output-directory outgoing doesn't already\nexist.  As a result, it's possible for patches from two different series\nto get mixed up if the user is not careful.  Fix the problem by\nunconditionally removing the output-directory before writing to it.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/log.c           | 5 +++++\n t/t4014-format-patch.sh | 9 +++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9e21232..babcfb7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -25,6 +25,7 @@\n #include \"version.h\"\n #include \"mailmap.h\"\n #include \"gpg-interface.h\"\n+#include \"dir.h\"\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n@@ -1312,8 +1313,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tsetup_pager();\n \n \tif (output_directory) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n \t\tif (use_stdout)\n \t\t\tdie(_(\"standard output, or directory, which one?\"));\n+\t\tstrbuf_addstr(&buf, output_directory);\n+\t\tremove_dir_recursively(&buf, 0);\n \t\tif (mkdir(output_directory, 0777) < 0 && errno != EEXIST)\n \t\t\tdie_errno(_(\"Could not create directory '%s'\"),\n \t\t\t\t  output_directory);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 58d4180..7ba0e66 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -271,6 +271,15 @@ test_expect_success 'multiple files' '\n \tls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch\n '\n \n+test_expect_success 'multiple files; unclean output-directory' '\n+\tmkdir -p patches &&\n+\t: >patches/fuzzle &&\n+\tgit checkout side &&\n+\tgit format-patch -o patches/ master &&\n+\tls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch &&\n+\ttest_path_is_missing patches/fuzzle\n+'\n+\n test_expect_success 'reroll count' '\n \trm -fr patches &&\n \tgit format-patch -o patches --cover-letter --reroll-count 4 master..side >list &&\n-- \n1.8.3.1.379.g74a2e74\n"},{"id":"220816","messageId":"20130614130910.GF23890@serenity.lan","threadId":"34139","inReplyTo":"1371213813-7925-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-14T13:10:26Z","receivedAt":"2013-06-14T13:10:26Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:\n> The following command\n> \n>   $ git format-patch -o outgoing master\n> \n> does not ensure that the output-directory outgoing doesn't already\n> exist.  As a result, it's possible for patches from two different series\n> to get mixed up if the user is not careful.  Fix the problem by\n> unconditionally removing the output-directory before writing to it.\n\nI don't think this is the correct behaviour.  I can think of cases where\nI would want to output multiple things into the same directory.\n\nIt may be better to issue a warning when this happens, or die and\nprovide a flag to let the user bypass that.\n"},{"id":"220818","messageId":"CALkWK0=dN4oqgvu9FL+EpqyekoQ385tTGbd3Gnf4mee-eTBeOA@mail.gmail.com","threadId":"34139","inReplyTo":"20130614130910.GF23890@serenity.lan","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-14T13:15:19Z","receivedAt":"2013-06-14T13:15:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"John Keeping wrote:\n> I don't think this is the correct behaviour.  I can think of cases where\n> I would want to output multiple things into the same directory.\n\nformat.cleanOutputDirectory = true|false?\n"},{"id":"220817","messageId":"20130614131629.GA1086@paksenarrion.iveqy.com","threadId":"34139","inReplyTo":"1371213813-7925-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-06-14T13:16:29Z","receivedAt":"2013-06-14T13:16:29Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"Hi,\njust some questions about your patch.\n\nOn Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:\n> The following command\n> \n>   $ git format-patch -o outgoing master\n> \n> does not ensure that the output-directory outgoing doesn't already\n> exist.  As a result, it's possible for patches from two different series\n> to get mixed up if the user is not careful.  Fix the problem by\n> unconditionally removing the output-directory before writing to it.\n\nI'm not entirely happy about removing untracked stuff without asking\nthe user. What if the output directory isn't empty? What if a user just\nwant them in ~?\n\nHowever I think this patch can improve the workflow for experienced\ndevelopers. Can we tweak this in some way to get the best out of both\nworlds?\n\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n>  \t\tif (use_stdout)\n>  \t\t\tdie(_(\"standard output, or directory, which one?\"));\n> +\t\tstrbuf_addstr(&buf, output_directory);\n> +\t\tremove_dir_recursively(&buf, 0);\n\nShould we have a strbuf_release here?\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"220823","messageId":"20130614132102.GG23890@serenity.lan","threadId":"34139","inReplyTo":"CALkWK0=dN4oqgvu9FL+EpqyekoQ385tTGbd3Gnf4mee-eTBeOA@mail.gmail.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-14T13:21:02Z","receivedAt":"2013-06-14T13:21:02Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote:\n> John Keeping wrote:\n> > I don't think this is the correct behaviour.  I can think of cases where\n> > I would want to output multiple things into the same directory.\n> \n> format.cleanOutputDirectory = true|false?\n\nMaybe, but I was thinking of something more like:\n\n    Output directory is not empty, use \"--allow-non-empty-dir\" if you\n    really want to proceed.\n\nUsing that configuration variable lets someone shoot themselves in the\nfoot quite badly if they forget that they have set it and set the output\ndirectory to somewhere containing important data.\n"},{"id":"220824","messageId":"CALkWK0nyNXsyzoTVz9vE5c5FP5k-KnPpzW2LryuCtmLP4WLXZA@mail.gmail.com","threadId":"34139","inReplyTo":"20130614131629.GA1086@paksenarrion.iveqy.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-14T13:34:18Z","receivedAt":"2013-06-14T13:34:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Fredrik Gustafsson wrote:\n> However I think this patch can improve the workflow for experienced\n> developers. Can we tweak this in some way to get the best out of both\n> worlds?\n\nThe main problem is that output-directory can be an absolute path\n(like ~, in the extreme case).  I'm not sure how to trade-off safety\nfor speed.  My main itch is that completion doesn't work with my fp:\n\n  alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing\n\n>> +             struct strbuf buf = STRBUF_INIT;\n>> +\n>>               if (use_stdout)\n>>                       die(_(\"standard output, or directory, which one?\"));\n>> +             strbuf_addstr(&buf, output_directory);\n>> +             remove_dir_recursively(&buf, 0);\n>\n> Should we have a strbuf_release here?\n\nYeah, my stupidity.\n"},{"id":"220826","messageId":"20130614134833.GB1086@paksenarrion.iveqy.com","threadId":"34139","inReplyTo":"CALkWK0nyNXsyzoTVz9vE5c5FP5k-KnPpzW2LryuCtmLP4WLXZA@mail.gmail.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-06-14T13:48:33Z","receivedAt":"2013-06-14T13:48:33Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:\n> Fredrik Gustafsson wrote:\n> > However I think this patch can improve the workflow for experienced\n> > developers. Can we tweak this in some way to get the best out of both\n> > worlds?\n> \n> The main problem is that output-directory can be an absolute path\n> (like ~, in the extreme case).  I'm not sure how to trade-off safety\n> for speed.  My main itch is that completion doesn't work with my fp:\n> \n>   alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing\n> \n\nIf I let myself to drift off a bit...\n\ngit format-patch always creates a new directory like:\n.git/outgoing/[patchname]<FROM commit short sha1>...<TO commit short sha1>\nand possible runs a custom command afterwards. Like cd to the patch\ndirectory, open the cover-letter in your editor etc.\n\ngit send-email without patches specified gives you a list of\npatch-series from .git/outgoing to choose from with the latest generated\npatchserie already selected.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"220827","messageId":"20130614135032.GA5394@goldbirke","threadId":"34139","inReplyTo":"CALkWK0nyNXsyzoTVz9vE5c5FP5k-KnPpzW2LryuCtmLP4WLXZA@mail.gmail.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-06-14T13:50:32Z","receivedAt":"2013-06-14T13:50:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:\n> My main itch is that completion doesn't work with my fp:\n> \n>   alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing\n\nWhy not define your custom completion function for this alias in your\n.bashrc?\n\n  _git_fp () { _git_format_patch ; }\n\n\nBest,\nGábor\n"},{"id":"220860","messageId":"CALkWK0k6qbTrqGgt_PVk=GN=bMaxVv9GDA=iHowRUbNjN+oLOg@mail.gmail.com","threadId":"34139","inReplyTo":"20130614135032.GA5394@goldbirke","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-14T13:51:59Z","receivedAt":"2013-06-14T13:51:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"SZEDER Gábor wrote:\n>   _git_fp () { _git_format_patch ; }\n\nGood stopgap, thanks.\n"},{"id":"220857","messageId":"CALkWK0kOJTVOC01_uygdSRaAPMBsSBo4+2WZUi4mqNt3F6D8LA@mail.gmail.com","threadId":"34139","inReplyTo":"20130614134833.GB1086@paksenarrion.iveqy.com","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-14T13:52:46Z","receivedAt":"2013-06-14T13:52:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Fredrik Gustafsson wrote:\n> git format-patch always creates a new directory like:\n> .git/outgoing/[patchname]<FROM commit short sha1>...<TO commit short sha1>\n> and possible runs a custom command afterwards. Like cd to the patch\n> directory, open the cover-letter in your editor etc.\n>\n> git send-email without patches specified gives you a list of\n> patch-series from .git/outgoing to choose from with the latest generated\n> patchserie already selected.\n\nExcellent.  I like the idea.  Let's see what the others have to say.\n"},{"id":"220863","messageId":"7vip1gssoh.fsf@alter.siamese.dyndns.org","threadId":"34139","inReplyTo":"20130614130910.GF23890@serenity.lan","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-14T13:55:26Z","receivedAt":"2013-06-14T13:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:\n>> The following command\n>> \n>>   $ git format-patch -o outgoing master\n>> \n>> does not ensure that the output-directory outgoing doesn't already\n>> exist.  As a result, it's possible for patches from two different series\n>> to get mixed up if the user is not careful.  Fix the problem by\n>> unconditionally removing the output-directory before writing to it.\n>\n> I don't think this is the correct behaviour.  I can think of cases where\n> I would want to output multiple things into the same directory.\n>\n> It may be better to issue a warning when this happens, or die and\n> provide a flag to let the user bypass that.\n\nAbsolutely.  I think this too dangeous, and will kill the\npossibility to further improve \"format-patch -v$n\".\n\nAn option (and configuration that expresses the user's wish) to\nerror out when -o diretory exists is probably as far as we would\nwant to go. Removal is way too much to accept without risk of\nhurting users.\n"},{"id":"220865","messageId":"7vehc4ssj9.fsf@alter.siamese.dyndns.org","threadId":"34139","inReplyTo":"20130614132102.GG23890@serenity.lan","subject":"Re: [PATCH] format-patch: remove existing output-directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-14T13:58:34Z","receivedAt":"2013-06-14T13:58:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote:\n>> John Keeping wrote:\n>> > I don't think this is the correct behaviour.  I can think of cases where\n>> > I would want to output multiple things into the same directory.\n>> \n>> format.cleanOutputDirectory = true|false?\n>\n> Maybe, but I was thinking of something more like:\n>\n>     Output directory is not empty, use \"--allow-non-empty-dir\" if you\n>     really want to proceed.\n>\n> Using that configuration variable lets someone shoot themselves in the\n> foot quite badly if they forget that they have set it and set the output\n> directory to somewhere containing important data.\n\nYes.  format.refuseToOutputToNonEmpty that defaults to false and\ngives your error message is a sensible way to go.\n"}]}