Re: [PATCH v4 1/2] format-patch: create leading components of output directory
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.
Re: [PATCH v4 1/2] format-patch: create leading components of output directory
On Fri, Oct 11, 2019 at 5:45 PM Bert Wesarg wrote: > > On Fri, Oct 11, 2019 at 4:46 PM SZEDER Gábor 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 > > >
Re: [PATCH v4 1/2] format-patch: create leading components of output directory
On Fri, Oct 11, 2019 at 4:46 PM SZEDER Gábor 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 > >
Re: [PATCH v4 1/2] format-patch: create leading components of output directory
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 >
[PATCH v4 1/2] format-patch: create leading components of output directory
'git format-patch -o ' did an equivalent of 'mkdir '
not 'mkdir -p ', 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
---
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
Cc: Junio C Hamano
Cc: SZEDER Gábor
---
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(-)
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 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 &&
--
2.23.0.13.g28bc381d7c
