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

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

[Ramkumar Ramachandra]

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(+)

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

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

[John Keeping]

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.

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

[Ramkumar Ramachandra]

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?

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

[Fredrik Gustafsson]

Hi,
just some questions about your patch.

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'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?

> +		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

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

[John Keeping]

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.

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

[Ramkumar Ramachandra]

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

>> +             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.

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

[Fredrik Gustafsson]

On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:
> 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

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

[SZEDER Gábor]

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

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

[Ramkumar Ramachandra]

SZEDER Gábor wrote:
>   _git_fp () { _git_format_patch ; }

Good stopgap, thanks.

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

[Ramkumar Ramachandra]

Fredrik Gustafsson wrote:
> 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.

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.