[v4,1/2] format-patch: create leading components of output directory
diff mbox series

Message ID b172eba0b748c3f0f638786a5cfba905aca385cc.1570782773.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • [v4,1/2] format-patch: create leading components of output directory
Related show

Commit Message

Bert Wesarg Oct. 11, 2019, 8:36 a.m. UTC
'git format-patch -o <outdir>' did an equivalent of 'mkdir <outdir>'
not 'mkdir -p <outdir>', which is being corrected.

Avoid the usage of 'adjust_shared_perm' on the leading directories which
may have security implications. Achieved by temporarily disabling of
'config.sharedRepository' like 'git init' does.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---

Changes in v2:
 * squashed and base new tests on 'dl/format-patch-doc-test-cleanup'

Changes in v3:
 * avoid applying adjust_shared_perm

Changes in v4:
 * based on dl/format-patch-doc-test-cleanup and adopt it

Cc: Denton Liu <liu.denton@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/config/format.txt    |  2 +-
 Documentation/git-format-patch.txt |  3 ++-
 builtin/log.c                      | 16 ++++++++++++++++
 t/t4014-format-patch.sh            | 23 +++++++++++++++++++++++
 4 files changed, 42 insertions(+), 2 deletions(-)

Comments

SZEDER Gábor Oct. 11, 2019, 2:46 p.m. UTC | #1
On Fri, Oct 11, 2019 at 10:36:41AM +0200, Bert Wesarg wrote:
> Changes in v4:
>  * based on dl/format-patch-doc-test-cleanup and adopt it

Thanks...  but here I am nitpicking again, sorry :)

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 72b09896cf..9facc3a79e 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1606,6 +1606,29 @@ test_expect_success 'From line has expected format' '
>  	test_cmp from filtered
>  '
>  
> +test_expect_success 'format-patch -o with no leading directories' '
> +	rm -fr patches &&
> +	git format-patch -o patches master..side &&
> +	count=$(git rev-list --count master..side) &&
> +	ls patches >list &&
> +	test_line_count = $count list
> +'
> +
> +test_expect_success 'format-patch -o with leading existing directories' '
> +	git format-patch -o patches/side master..side &&

The previous test case creates the 'patches' directory and leaves it
behind, and this test implicitly relies on that directory to check
that 'format-patch -o' can deal with already existing directories.  So
if the previous test case were to fail early or were not run at all
(e.g. './t4014-format-patch.sh -r 1,137'), then that directory
wouldn't exist, and, consequently, this test case would not check what
it was supposed to.

I think it would be better to be more explicit and self-contained
about it, and create a leading directory in this test case:

   mkdir existing-dir &&
   git format-patch -o existing-dir/side master..size &&
   ls existing-dir/side >list &&

> +	count=$(git rev-list --count master..side) &&
> +	ls patches/side >list &&
> +	test_line_count = $count list
> +'
> +
> +test_expect_success 'format-patch -o with leading non-existing directories' '
> +	rm -fr patches &&
> +	git format-patch -o patches/side master..side &&
> +	count=$(git rev-list --count master..side) &&
> +	ls patches/side >list &&
> +	test_line_count = $count list
> +'
> +
>  test_expect_success 'format-patch format.outputDirectory option' '
>  	test_config format.outputDirectory patches &&
>  	rm -fr patches &&
> -- 
> 2.23.0.13.g28bc381d7c
>
Bert Wesarg Oct. 11, 2019, 3:45 p.m. UTC | #2
On Fri, Oct 11, 2019 at 4:46 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 10:36:41AM +0200, Bert Wesarg wrote:
> > Changes in v4:
> >  * based on dl/format-patch-doc-test-cleanup and adopt it
>
> Thanks...  but here I am nitpicking again, sorry :)
>
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 72b09896cf..9facc3a79e 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -1606,6 +1606,29 @@ test_expect_success 'From line has expected format' '
> >       test_cmp from filtered
> >  '
> >
> > +test_expect_success 'format-patch -o with no leading directories' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches master..side &&
> > +     count=$(git rev-list --count master..side) &&
> > +     ls patches >list &&
> > +     test_line_count = $count list
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading existing directories' '
> > +     git format-patch -o patches/side master..side &&
>
> The previous test case creates the 'patches' directory and leaves it
> behind, and this test implicitly relies on that directory to check
> that 'format-patch -o' can deal with already existing directories.  So
> if the previous test case were to fail early or were not run at all
> (e.g. './t4014-format-patch.sh -r 1,137'), then that directory
> wouldn't exist, and, consequently, this test case would not check what
> it was supposed to.
>
> I think it would be better to be more explicit and self-contained
> about it, and create a leading directory in this test case:
>
>    mkdir existing-dir &&
>    git format-patch -o existing-dir/side master..size &&
>    ls existing-dir/side >list &&
>

thanks. Your nitpicking is always appreciated.

Bert

> > +     count=$(git rev-list --count master..side) &&
> > +     ls patches/side >list &&
> > +     test_line_count = $count list
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading non-existing directories' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches/side master..side &&
> > +     count=$(git rev-list --count master..side) &&
> > +     ls patches/side >list &&
> > +     test_line_count = $count list
> > +'
> > +
> >  test_expect_success 'format-patch format.outputDirectory option' '
> >       test_config format.outputDirectory patches &&
> >       rm -fr patches &&
> > --
> > 2.23.0.13.g28bc381d7c
> >
Bert Wesarg Oct. 11, 2019, 3:47 p.m. UTC | #3
On Fri, Oct 11, 2019 at 5:45 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> On Fri, Oct 11, 2019 at 4:46 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 10:36:41AM +0200, Bert Wesarg wrote:
> > > Changes in v4:
> > >  * based on dl/format-patch-doc-test-cleanup and adopt it
> >
> > Thanks...  but here I am nitpicking again, sorry :)
> >
> > > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > > index 72b09896cf..9facc3a79e 100755
> > > --- a/t/t4014-format-patch.sh
> > > +++ b/t/t4014-format-patch.sh
> > > @@ -1606,6 +1606,29 @@ test_expect_success 'From line has expected format' '
> > >       test_cmp from filtered
> > >  '
> > >
> > > +test_expect_success 'format-patch -o with no leading directories' '
> > > +     rm -fr patches &&
> > > +     git format-patch -o patches master..side &&
> > > +     count=$(git rev-list --count master..side) &&
> > > +     ls patches >list &&
> > > +     test_line_count = $count list
> > > +'
> > > +
> > > +test_expect_success 'format-patch -o with leading existing directories' '
> > > +     git format-patch -o patches/side master..side &&
> >
> > The previous test case creates the 'patches' directory and leaves it
> > behind, and this test implicitly relies on that directory to check
> > that 'format-patch -o' can deal with already existing directories.  So
> > if the previous test case were to fail early or were not run at all
> > (e.g. './t4014-format-patch.sh -r 1,137'), then that directory
> > wouldn't exist, and, consequently, this test case would not check what
> > it was supposed to.
> >
> > I think it would be better to be more explicit and self-contained
> > about it, and create a leading directory in this test case:
> >
> >    mkdir existing-dir &&
> >    git format-patch -o existing-dir/side master..size &&
> >    ls existing-dir/side >list &&

one question: How about removing this directory first, just to be
sure, that mkdir does create a directory?

Bert

> >
>
> thanks. Your nitpicking is always appreciated.
>
> Bert
>
> > > +     count=$(git rev-list --count master..side) &&
> > > +     ls patches/side >list &&
> > > +     test_line_count = $count list
> > > +'
> > > +
> > > +test_expect_success 'format-patch -o with leading non-existing directories' '
> > > +     rm -fr patches &&
> > > +     git format-patch -o patches/side master..side &&
> > > +     count=$(git rev-list --count master..side) &&
> > > +     ls patches/side >list &&
> > > +     test_line_count = $count list
> > > +'
> > > +
> > >  test_expect_success 'format-patch format.outputDirectory option' '
> > >       test_config format.outputDirectory patches &&
> > >       rm -fr patches &&
> > > --
> > > 2.23.0.13.g28bc381d7c
> > >
SZEDER Gábor Oct. 11, 2019, 4:17 p.m. UTC | #4
On Fri, Oct 11, 2019 at 05:47:44PM +0200, Bert Wesarg wrote:
> > > > +test_expect_success 'format-patch -o with no leading directories' '
> > > > +     rm -fr patches &&
> > > > +     git format-patch -o patches master..side &&
> > > > +     count=$(git rev-list --count master..side) &&
> > > > +     ls patches >list &&
> > > > +     test_line_count = $count list
> > > > +'
> > > > +
> > > > +test_expect_success 'format-patch -o with leading existing directories' '
> > > > +     git format-patch -o patches/side master..side &&
> > >
> > > The previous test case creates the 'patches' directory and leaves it
> > > behind, and this test implicitly relies on that directory to check
> > > that 'format-patch -o' can deal with already existing directories.  So
> > > if the previous test case were to fail early or were not run at all
> > > (e.g. './t4014-format-patch.sh -r 1,137'), then that directory
> > > wouldn't exist, and, consequently, this test case would not check what
> > > it was supposed to.
> > >
> > > I think it would be better to be more explicit and self-contained
> > > about it, and create a leading directory in this test case:
> > >
> > >    mkdir existing-dir &&
> > >    git format-patch -o existing-dir/side master..size &&
> > >    ls existing-dir/side >list &&
> 
> one question: How about removing this directory first, just to be
> sure, that mkdir does create a directory?

I'm not sure I understand...

Do you mean that a previous test might have already created and left a
directory with the same name behind, and then this 'mkdir' would error
out and thus fail the test?  If yes, then you're right with your
nitpicking on my nitpicking ;)  Though instead of 'rm -rf'ing that
directory I would suggest 'mkdir -p' to simply ignore it if it were to
exist.

Patch
diff mbox series

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index cb629fa769..40cad9278f 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -81,7 +81,7 @@  format.coverLetter::
 
 format.outputDirectory::
 	Set a custom directory to store the resulting files instead of the
-	current working directory.
+	current working directory. All directory components will be created.
 
 format.useAutoBase::
 	A boolean value which lets you enable the `--base=auto` option of
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 0ac56f4b70..2035d4d5d5 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -66,7 +66,8 @@  they are created in the current working directory. The default path
 can be set with the `format.outputDirectory` configuration option.
 The `-o` option takes precedence over `format.outputDirectory`.
 To store patches in the current working directory even when
-`format.outputDirectory` points elsewhere, use `-o .`.
+`format.outputDirectory` points elsewhere, use `-o .`. All directory
+components will be created.
 
 By default, the subject of a single patch is "[PATCH] " followed by
 the concatenation of lines from the commit message up to the first blank
diff --git a/builtin/log.c b/builtin/log.c
index 44b10b3415..8d08632858 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1765,10 +1765,26 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		int saved;
 		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
 			rev.diffopt.use_color = GIT_COLOR_NEVER;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
+		/*
+		 * We consider <outdir> as 'outside of gitdir', therefore avoid
+		 * applying adjust_shared_perm in s-c-l-d.
+		 */
+		saved = get_shared_repository();
+		set_shared_repository(0);
+		switch (safe_create_leading_directories_const(output_directory)) {
+		case SCLD_OK:
+		case SCLD_EXISTS:
+			break;
+		default:
+			die(_("could not create leading directories "
+			      "of '%s'"), output_directory);
+		}
+		set_shared_repository(saved);
 		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 72b09896cf..9facc3a79e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1606,6 +1606,29 @@  test_expect_success 'From line has expected format' '
 	test_cmp from filtered
 '
 
+test_expect_success 'format-patch -o with no leading directories' '
+	rm -fr patches &&
+	git format-patch -o patches master..side &&
+	count=$(git rev-list --count master..side) &&
+	ls patches >list &&
+	test_line_count = $count list
+'
+
+test_expect_success 'format-patch -o with leading existing directories' '
+	git format-patch -o patches/side master..side &&
+	count=$(git rev-list --count master..side) &&
+	ls patches/side >list &&
+	test_line_count = $count list
+'
+
+test_expect_success 'format-patch -o with leading non-existing directories' '
+	rm -fr patches &&
+	git format-patch -o patches/side master..side &&
+	count=$(git rev-list --count master..side) &&
+	ls patches/side >list &&
+	test_line_count = $count list
+'
+
 test_expect_success 'format-patch format.outputDirectory option' '
 	test_config format.outputDirectory patches &&
 	rm -fr patches &&