| Message ID | b172eba0b748c3f0f638786a5cfba905aca385cc.1570782773.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] format-patch: create leading components of output directory | expand |
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 >
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 > >
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 > > >
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.
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 &&
'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(-)