Thanks for reviving this abandoned patch. Please see review comments below, some of which repeat comments from the previous attempt[1], and some of which are new. Most of the new ones are minor, although there is at least one major problem. On Sat, Jan 9, 2016 at 9:30 PM, Stephen P. Smith <ischis2@xxxxxxx> wrote: > From: Alexander Kuleshov <kuleshovmail@xxxxxxxxx> > > We can pass -o/--output-directory to the format-patch command to > store patches not in the working directory. This patch introduces s/not in/in some place other than/ > format.outputDirectory configuration option for same purpose. > > The case of usage of this configuration option can be convinience >From [1]: s/convinience/convenience/ > to not pass everytime -o/--output-directory if an user has pattern Also[1]: s/everytime/every time/ or: s/everytime/each time/ > to store all patches in the /patches directory for example. > > The format.outputDirectory has lower priority than command line > option, so if user will set format.outputDirectory and pass the > command line option, a result will be stored in a directory that > passed to command line option. > > Signed-off-by: Alexander Kuleshov <kuleshovmail@xxxxxxxxx> > Signed-off-by: Stephen P. Smith <ischis2@xxxxxxx> [1]: http://article.gmane.org/gmane.comp.version-control.git/272199 > --- > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > @@ -57,7 +57,11 @@ The names of the output files are printed to standard > output, unless the `--stdout` option is specified. > > If `-o` is specified, output files are created in <dir>. Otherwise > -they are created in the current working directory. > +they are created in the current working directory. The default path > +can be set with the setting 'format.outputDirectory' configuration option. s/setting// > +If `-o` is specified and 'format.outputDirectory' is set, output files > +will be stored in a <dir> that passed to `-o`. When 'format.outputDirectory' > +is set, to get default behaviour back is to pass './' to the `-o`. This is difficult to read. How about replacing these two sentences with something like this: The `-o` option takes precedence over `format.outputDirectory`. To store patches in the current working directory even when `format.outputDirectory` points elsewhere, use `-o .`. > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > @@ -40,6 +40,19 @@ test_expect_success setup ' Rather than adding new tests at the very top of the script, it's more common to add them to the bottom or at least to insert them after other similar tests. > +test_expect_success "format-patch format.outputDirectory option" ' Use single- rather than double-quotes: s/"/'/ > + test_config format.outputDirectory "patches/" && We can drop the unnecessary quotes around "patches". Also, can we drop the unnecessary "/"? > + git format-patch master..side && Since this test is about verifying that the "patches" directory got created and used, you want to be more careful about ensuring that detritus from preceding tests won't muck up your results; for instance, if an earlier test had also used a directory named "patches" and had dumped 42 files there instead of the 3 expected by this test. Therefore, you should insert "rm -fr patches &&" before the git-format-patch invocation. > + cnt=$(ls patches | wc -l) && > + test $cnt = 3 Periodically, we have trouble with the output of "wc -l" on Mac OS X since the output has leading spaces. This code doesn't trip over that problem since it doesn't quote the output, but it still feels fragile to be comparing the it against a number using the string equality test '='. How about using the '-eq' numeric equality test instead? Moreover, there is no need for the temporary 'cnt' variable. Instead: test $(ls patches | wc -l) -eq 3 && Depending upon taste, you might alternately use: ls patches >actual && test_line_count = 3 actual which would give you more useful debugging output upon failure. > +' > + > +test_expect_success "format-patch format.outputDirectory overwritten with -o" ' Use single- rather than double-qoutes: s/"/'/ Also, how about rewording it? 'format-patch -o overrides format.outputDirectory' > + test_config format.outputDirectory "patches/" && Style: drop unnecessary quotes around "patches" Style: drop unnecessary "/" > + git format-patch master..side -o "." && Style: drop unnecessary quotes around "." > + test_path_is_missing patches/ Style: drop unnecessary "/" There is a rather severe problem with this test in that it fails unconditionally. It wants to verify that -o takes precedence over format.outputDirectory by checking that the directory "patches" did not get created by git-format-patch, however, that directory already exists since it was created by the previous test, thus test_path_is_missing() fails. Therefore, you should insert "rm -fr patches &&" before the git-format-patch invocation. It also might not hurt to make the test a bit more robust by verifying not only that the directory specified by format.outputDirectory did not get created, but that the directory named by -o did get created, which means giving -o an argument other than ".". So, the final test might look like this: test_config format.outputDirectory patches && rm -fr patches patchset && git format-patch master..side -o patchset && test_path_is_missing patches && test_path_is_dir patchset > +' > -- > 2.7.0-rc2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html