threads / patch / 34139

patchformat-patch: remove existing output-directory

Subject: [PATCH] format-patch: remove existing output-directory

## tl;dr

12 messages between Jun 14, 2013 and Jun 14, 2013. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Ramkumar Ramachandra· Jun 14, 2013, 12:43 UTC · lore
The following command
  $ git format-patch -o outgoing master

does not ensure that the output-directory outgoing doesn't already exist. As a result, it's possible for patches from two different series to get mixed up if the user is not careful. Fix the problem by unconditionally removing the output-directory before writing to it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/log.c           | 5 +++++
 t/t4014-format-patch.sh | 9 +++++++++
 2 files changed, 14 insertions(+)
Show changes to 2 files +14 −0

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

diff --git a/builtin/log.c b/builtin/log.c
index 9e21232..babcfb7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,6 +25,7 @@
 #include "version.h"
 #include "mailmap.h"
 #include "gpg-interface.h"
+#include "dir.h"
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -1312,8 +1313,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		struct strbuf buf = STRBUF_INIT;
+
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
+		strbuf_addstr(&buf, output_directory);
+		remove_dir_recursively(&buf, 0);
 		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
 			die_errno(_("Could not create directory '%s'"),
 				  output_directory);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 58d4180..7ba0e66 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -271,6 +271,15 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
+test_expect_success 'multiple files; unclean output-directory' '
+	mkdir -p patches &&
+	: >patches/fuzzle &&
+	git checkout side &&
+	git format-patch -o patches/ master &&
+	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch &&
+	test_path_is_missing patches/fuzzle
+'
+
 test_expect_success 'reroll count' '
 	rm -fr patches &&
 	git format-patch -o patches --cover-letter --reroll-count 4 master..side >list &&

-- 
1.8.3.1.379.g74a2e74
John Keeping· Jun 14, 2013, 13:10 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] format-patch: remove existing output-directory

On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:
Show 8 quoted lines
> The following command
> 
>   $ git format-patch -o outgoing master
> 
> does not ensure that the output-directory outgoing doesn't already
> exist.  As a result, it's possible for patches from two different series
> to get mixed up if the user is not careful.  Fix the problem by
> unconditionally removing the output-directory before writing to it.

I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory.

It may be better to issue a warning when this happens, or die and provide a flag to let the user bypass that.

Ramkumar Ramachandra· Jun 14, 2013, 13:15 UTC · re: John Keeping · lore

Re: [PATCH] format-patch: remove existing output-directory

John Keeping wrote:
> I don't think this is the correct behaviour.  I can think of cases where
> I would want to output multiple things into the same directory.
format.cleanOutputDirectory = true|false?
John Keeping· Jun 14, 2013, 13:21 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] format-patch: remove existing output-directory

On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote:
Show 5 quoted lines
> John Keeping wrote:
> > I don't think this is the correct behaviour.  I can think of cases where
> > I would want to output multiple things into the same directory.
> 
> format.cleanOutputDirectory = true|false?
Maybe, but I was thinking of something more like:
    Output directory is not empty, use "--allow-non-empty-dir" if you
    really want to proceed.

Using that configuration variable lets someone shoot themselves in the foot quite badly if they forget that they have set it and set the output directory to somewhere containing important data.

Junio C Hamano· Jun 14, 2013, 13:58 UTC · re: John Keeping · lore

Re: [PATCH] format-patch: remove existing output-directory

John Keeping <john@keeping.me.uk> writes:
Show 15 quoted lines
> On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote:
>> John Keeping wrote:
>> > I don't think this is the correct behaviour.  I can think of cases where
>> > I would want to output multiple things into the same directory.
>> 
>> format.cleanOutputDirectory = true|false?
>
> Maybe, but I was thinking of something more like:
>
>     Output directory is not empty, use "--allow-non-empty-dir" if you
>     really want to proceed.
>
> Using that configuration variable lets someone shoot themselves in the
> foot quite badly if they forget that they have set it and set the output
> directory to somewhere containing important data.

Yes. format.refuseToOutputToNonEmpty that defaults to false and gives your error message is a sensible way to go.

Junio C Hamano· Jun 14, 2013, 13:55 UTC · re: John Keeping · lore

Re: [PATCH] format-patch: remove existing output-directory

John Keeping <john@keeping.me.uk> writes:
Show 15 quoted lines
> On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:
>> The following command
>> 
>>   $ git format-patch -o outgoing master
>> 
>> does not ensure that the output-directory outgoing doesn't already
>> exist.  As a result, it's possible for patches from two different series
>> to get mixed up if the user is not careful.  Fix the problem by
>> unconditionally removing the output-directory before writing to it.
>
> I don't think this is the correct behaviour.  I can think of cases where
> I would want to output multiple things into the same directory.
>
> It may be better to issue a warning when this happens, or die and
> provide a flag to let the user bypass that.

Absolutely. I think this too dangeous, and will kill the possibility to further improve "format-patch -v$n".

An option (and configuration that expresses the user's wish) to error out when -o diretory exists is probably as far as we would want to go. Removal is way too much to accept without risk of hurting users.

Fredrik Gustafsson· Jun 14, 2013, 13:16 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] format-patch: remove existing output-directory

Hi, just some questions about your patch.

On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote:
Show 8 quoted lines
> The following command
> 
>   $ git format-patch -o outgoing master
> 
> does not ensure that the output-directory outgoing doesn't already
> exist.  As a result, it's possible for patches from two different series
> to get mixed up if the user is not careful.  Fix the problem by
> unconditionally removing the output-directory before writing to it.

I'm not entirely happy about removing untracked stuff without asking the user. What if the output directory isn't empty? What if a user just want them in ~?

However I think this patch can improve the workflow for experienced developers. Can we tweak this in some way to get the best out of both worlds?

Show 6 quoted lines
> +		struct strbuf buf = STRBUF_INIT;
> +
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
> +		strbuf_addstr(&buf, output_directory);
> +		remove_dir_recursively(&buf, 0);
Should we have a strbuf_release here?
-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com
Ramkumar Ramachandra· Jun 14, 2013, 13:34 UTC · re: Fredrik Gustafsson · lore

Re: [PATCH] format-patch: remove existing output-directory

Fredrik Gustafsson wrote:
> However I think this patch can improve the workflow for experienced
> developers. Can we tweak this in some way to get the best out of both
> worlds?

The main problem is that output-directory can be an absolute path (like ~, in the extreme case). I'm not sure how to trade-off safety for speed. My main itch is that completion doesn't work with my fp:

  alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing
Show 8 quoted lines
>> +             struct strbuf buf = STRBUF_INIT;
>> +
>>               if (use_stdout)
>>                       die(_("standard output, or directory, which one?"));
>> +             strbuf_addstr(&buf, output_directory);
>> +             remove_dir_recursively(&buf, 0);
>
> Should we have a strbuf_release here?
Yeah, my stupidity.
Fredrik Gustafsson· Jun 14, 2013, 13:48 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] format-patch: remove existing output-directory

On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:
Show 11 quoted lines
> Fredrik Gustafsson wrote:
> > However I think this patch can improve the workflow for experienced
> > developers. Can we tweak this in some way to get the best out of both
> > worlds?
> 
> The main problem is that output-directory can be an absolute path
> (like ~, in the extreme case).  I'm not sure how to trade-off safety
> for speed.  My main itch is that completion doesn't work with my fp:
> 
>   alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing
> 
If I let myself to drift off a bit...

git format-patch always creates a new directory like: .git/outgoing/[patchname]<FROM commit short sha1>...<TO commit short sha1> and possible runs a custom command afterwards. Like cd to the patch directory, open the cover-letter in your editor etc.

git send-email without patches specified gives you a list of patch-series from .git/outgoing to choose from with the latest generated patchserie already selected.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com
Ramkumar Ramachandra· Jun 14, 2013, 13:52 UTC · re: Fredrik Gustafsson · lore

Re: [PATCH] format-patch: remove existing output-directory

Fredrik Gustafsson wrote:
Show 8 quoted lines
> git format-patch always creates a new directory like:
> .git/outgoing/[patchname]<FROM commit short sha1>...<TO commit short sha1>
> and possible runs a custom command afterwards. Like cd to the patch
> directory, open the cover-letter in your editor etc.
>
> git send-email without patches specified gives you a list of
> patch-series from .git/outgoing to choose from with the latest generated
> patchserie already selected.
Excellent.  I like the idea.  Let's see what the others have to say.
SZEDER Gábor· Jun 14, 2013, 13:50 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] format-patch: remove existing output-directory

On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:
> My main itch is that completion doesn't work with my fp:
> 
>   alias.fp = !rm -rf outgoing && git format-patch -M -C -o outgoing

Why not define your custom completion function for this alias in your .bashrc?

  _git_fp () { _git_format_patch ; }

Best, Gábor

← back to recent threads