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

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

Commit Message

Bert Wesarg Oct. 5, 2019, 8:43 a.m. UTC
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

--- 

ChangeLog:

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

Cc: Denton Liu <liu.denton@gmail.com>
---
 Documentation/config/format.txt    |  2 +-
 Documentation/git-format-patch.txt |  3 ++-
 builtin/log.c                      |  8 ++++++++
 t/t4014-format-patch.sh            | 20 ++++++++++++++++++++
 4 files changed, 31 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Oct. 6, 2019, 12:57 a.m. UTC | #1
Bert Wesarg <bert.wesarg@googlemail.com> writes:

> +		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);
> +		}
>  		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
>  			die_errno(_("could not create directory '%s'"),
>  				  output_directory);

There is a slight discrepancy here in that mkdir(..., 0777) is to
honor the umask setting of the user who is running the command and
does not care about anybody else being able to (or unable to) access
the resulting directory.  On the other hand, s-c-l-d is (as you can
guess from the location the function is defined, sha1-file.c) meant
to be used to create hierarchy _inside_ $GIT_DIR/ in such a way that
anybody who needs to access the repository can access it (via
core.sharedrepository config).

I do not think it matters too much in practice, but

	$ git format-patch -o $HOME/my/patch/depot

that creates intermediate levels that can be writable by other
users, only because the repository you took the patches from was
shared with other users, may probably be seen as a security bug.
SZEDER Gábor Oct. 7, 2019, 9:03 p.m. UTC | #2
On Sat, Oct 05, 2019 at 10:43:51AM +0200, Bert Wesarg wrote:
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 83f52614d3..2f2cd6fea6 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1606,6 +1606,26 @@ 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 &&
> +	git rev-list master..side >list &&
> +	test_line_count = $(ls patches | wc -l) list

This is sort of a nit...

So, these tests check that 'git rev-list ...' lists as many commits as
the number of files created by 'git format-patch'.  While it doesn't
affect the tests' correctness, this is subtly different from checking
that 'git format-patch' created as many files as the number of commits
listed by 'git rev-list'.

Consider how the tests' output would look like on failure:
'test_line_count' shows an error message that includes the content of
the file to be checked, which in this case would consist of a bunch of
commit object ids:

  test_line_count: line count for list != 3
  f7af51d27933a90554b6e9212a7e5d4ad1c74569
  bd89fce9f5096eb5cad67c342b40818b7e3ce9e4

On one hand, these object ids won't mean much to anyone who might have
to debug such a test failure in the future, and on the other these
tests are about 'git format-patch', not about 'git rev-list'.  If the
check were written like this:

  count=$(git rev-list --count master..side) &&
  ls patches >list &&
  test_line_count = $count list

then the error message on failure would look something like this:

  test_line_count: line count for list != 3
  0001-first.patch
  0002-second.patch

which, I think, would be more useful.


> +'
> +
> +test_expect_success 'format-patch -o with leading existing directories' '
> +	git format-patch -o patches/side master..side &&
> +	git rev-list master..side >list &&
> +	test_line_count = $(ls patches/side | wc -l) list
> +'
> +
> +test_expect_success 'format-patch -o with leading non-existing directories' '
> +	rm -fr patches &&
> +	git format-patch -o patches/side master..side &&
> +	git rev-list master..side >list &&
> +	test_line_count = $(ls patches/side | wc -l) list
> +'
> +
>  test_expect_success 'format-patch format.outputDirectory option' '
>  	test_config format.outputDirectory patches &&
>  	rm -fr patches &&
> -- 
> 2.23.0.11.g242cf7f110
>
Junio C Hamano Oct. 8, 2019, 3:23 a.m. UTC | #3
SZEDER Gábor <szeder.dev@gmail.com> writes:

> On one hand, these object ids won't mean much to anyone who might have
> to debug such a test failure in the future, and on the other these
> tests are about 'git format-patch', not about 'git rev-list'.  If the
> check were written like this:
>
>   count=$(git rev-list --count master..side) &&
>   ls patches >list &&
>   test_line_count = $count list
>
> then the error message on failure would look something like this:
>
>   test_line_count: line count for list != 3
>   0001-first.patch
>   0002-second.patch
>
> which, I think, would be more useful.

That's nice attention to the detail ;-)
Bert Wesarg Oct. 8, 2019, 9:06 a.m. UTC | #4
On Mon, Oct 7, 2019 at 11:03 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Sat, Oct 05, 2019 at 10:43:51AM +0200, Bert Wesarg wrote:
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 83f52614d3..2f2cd6fea6 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -1606,6 +1606,26 @@ 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 &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches | wc -l) list
>
> This is sort of a nit...
>
> So, these tests check that 'git rev-list ...' lists as many commits as
> the number of files created by 'git format-patch'.  While it doesn't
> affect the tests' correctness, this is subtly different from checking
> that 'git format-patch' created as many files as the number of commits
> listed by 'git rev-list'.
>
> Consider how the tests' output would look like on failure:
> 'test_line_count' shows an error message that includes the content of
> the file to be checked, which in this case would consist of a bunch of
> commit object ids:
>
>   test_line_count: line count for list != 3
>   f7af51d27933a90554b6e9212a7e5d4ad1c74569
>   bd89fce9f5096eb5cad67c342b40818b7e3ce9e4
>
> On one hand, these object ids won't mean much to anyone who might have
> to debug such a test failure in the future, and on the other these
> tests are about 'git format-patch', not about 'git rev-list'.  If the
> check were written like this:
>
>   count=$(git rev-list --count master..side) &&
>   ls patches >list &&
>   test_line_count = $count list
>
> then the error message on failure would look something like this:
>
>   test_line_count: line count for list != 3
>   0001-first.patch
>   0002-second.patch
>
> which, I think, would be more useful.

thanks for this detail. As I copied an existing test with this
pattern, I will add a new pre-patch to this serires which applies your
idea to the existing test first, before I add new tests for this
patch.

Bert

>
>
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading existing directories' '
> > +     git format-patch -o patches/side master..side &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches/side | wc -l) list
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading non-existing directories' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches/side master..side &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches/side | wc -l) list
> > +'
> > +
> >  test_expect_success 'format-patch format.outputDirectory option' '
> >       test_config format.outputDirectory patches &&
> >       rm -fr patches &&
> > --
> > 2.23.0.11.g242cf7f110
> >

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 c4b35fdaf9..294534aacb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1770,6 +1770,14 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.diffopt.use_color = GIT_COLOR_NEVER;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
+		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);
+		}
 		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 83f52614d3..2f2cd6fea6 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1606,6 +1606,26 @@  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 &&
+	git rev-list master..side >list &&
+	test_line_count = $(ls patches | wc -l) list
+'
+
+test_expect_success 'format-patch -o with leading existing directories' '
+	git format-patch -o patches/side master..side &&
+	git rev-list master..side >list &&
+	test_line_count = $(ls patches/side | wc -l) list
+'
+
+test_expect_success 'format-patch -o with leading non-existing directories' '
+	rm -fr patches &&
+	git format-patch -o patches/side master..side &&
+	git rev-list master..side >list &&
+	test_line_count = $(ls patches/side | wc -l) list
+'
+
 test_expect_success 'format-patch format.outputDirectory option' '
 	test_config format.outputDirectory patches &&
 	rm -fr patches &&