* [PATCH 2/3] format-patch: create output directory including leading components
2019-10-02 21:26 [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Bert Wesarg
@ 2019-10-02 21:26 ` Bert Wesarg
2019-10-03 0:54 ` Junio C Hamano
2019-10-02 21:26 ` [RFC PATCH 3/3] format-patch: use a command to generate the output directory name Bert Wesarg
2019-10-02 21:47 ` [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Denton Liu
2 siblings, 1 reply; 14+ messages in thread
From: Bert Wesarg @ 2019-10-02 21:26 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Documentation/config/format.txt | 3 +--
Documentation/git-format-patch.txt | 5 ++---
builtin/log.c | 8 ++++++++
t/t4014-format-patch.sh | 5 +++--
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index e17c5d6b0f..b6c96ece04 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -80,8 +80,7 @@ format.coverLetter::
format.outputDirectory::
Set a custom directory to store the resulting files instead of the
- current working directory. Only the trailing directory will be created
- though.
+ 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 fe7492353e..f418f490aa 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -66,9 +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 .`. Note that only
-the trailing directory will be created by Git, leading directories must
-already exists.
+`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..1ab9eb6b78 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1769,6 +1769,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 bf2715a503..43d608aa94 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1643,9 +1643,10 @@ test_expect_success 'format-patch -o with leading existing directories' '
test $(git rev-list master..side | wc -l) -eq $(ls patches/side | wc -l)
'
-test_expect_failure 'format-patch -o with leading non-existing directories' '
+test_expect_success 'format-patch -o with leading non-existing directories' '
rm -fr patches &&
- git format-patch -o patches/side master..side
+ git format-patch -o patches/side master..side &&
+ test $(git rev-list master..side | wc -l) -eq $(ls patches/side | wc -l)
'
test_expect_success 'format-patch format.outputDirectory option' '
--
2.23.0.11.g242cf7f110
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC PATCH 3/3] format-patch: use a command to generate the output directory name
2019-10-02 21:26 [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Bert Wesarg
2019-10-02 21:26 ` [PATCH 2/3] format-patch: create output directory including leading components Bert Wesarg
@ 2019-10-02 21:26 ` Bert Wesarg
2019-10-03 0:57 ` Junio C Hamano
2019-10-05 8:43 ` [PATCH v2 1/2] format-patch: create leading components of output directory Bert Wesarg
2019-10-02 21:47 ` [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Denton Liu
2 siblings, 2 replies; 14+ messages in thread
From: Bert Wesarg @ 2019-10-02 21:26 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Alexander Kuleshov, Eric Sunshine
Having 'format.outputDirectory' is convenient, but being able to process
all produced patches via a wildcard command is even more so. I.e.,
using an argument like '<dir>/*'. Neither '-o' nor
'format.outputDirectory' can be parameterized to produce a new unique
directory. Thus provide the new 'format.outputDirectoryCmd' configuration
to specify a command which does the job and puts the name to standard
output.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Cc: Alexander Kuleshov <kuleshovmail@gmail.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/config/format.txt | 5 +++++
Documentation/git-format-patch.txt | 6 +++++-
builtin/log.c | 24 +++++++++++++++++++++++-
t/t4014-format-patch.sh | 24 ++++++++++++++++++++++++
4 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index b6c96ece04..dcce2c67ef 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -82,6 +82,11 @@ format.outputDirectory::
Set a custom directory to store the resulting files instead of the
current working directory. All directory components will be created.
+format.outputDirectoryCmd::
+ The command which is used to name a custom directory to store the
+ resulting files instead of the current working directory. All directory
+ components will be created.
+
format.useAutoBase::
A boolean value which lets you enable the `--base=auto` option of
format-patch by default.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index f418f490aa..0da904255b 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -67,7 +67,11 @@ 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 .`. All directory
-components will be created.
+components will be created. The 'format.outputDirectoryCmd' configuration can
+be used to name a command to produce the directory name programmatically. The
+command should produce the name to its standard output. The
+`format.outputDirectory` configuration takes precedence over
+`format.outputDirectoryCmd`.
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 1ab9eb6b78..b102e86bea 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -774,6 +774,7 @@ static const char *signature = git_version_string;
static const char *signature_file;
static int config_cover_letter;
static const char *config_output_directory;
+static const char *config_output_directory_cmd;
enum {
COVER_UNSET,
@@ -856,6 +857,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
}
if (!strcmp(var, "format.outputdirectory"))
return git_config_string(&config_output_directory, var, value);
+ if (!strcmp(var, "format.outputdirectorycmd"))
+ return git_config_string(&config_output_directory_cmd, var, value);
if (!strcmp(var, "format.useautobase")) {
base_auto = git_config_bool(var, value);
return 0;
@@ -1756,8 +1759,27 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (rev.show_notes)
init_display_notes(&rev.notes_opt);
- if (!output_directory && !use_stdout)
+ if (!output_directory && !use_stdout) {
+ // outputDirectoryCmd can be preceeded by outputDirectory
+ if (!config_output_directory && config_output_directory_cmd) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ const char *argv[1];
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ argv[0] = config_output_directory_cmd;
+ cp.argv = argv;
+ cp.use_shell = 1;
+ rc = capture_command(&cp, &buf, PATH_MAX);
+ if (rc)
+ die(_("outputDirectoryCmd command failed: "
+ "'%s'"), config_output_directory_cmd);
+ strbuf_setlen(&buf, strcspn(buf.buf, "\r\n"));
+ config_output_directory = strbuf_detach(&buf, NULL);
+ }
+
output_directory = config_output_directory;
+ }
if (!use_stdout)
output_directory = set_outdir(prefix, output_directory);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 43d608aa94..bf2547ce87 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1664,6 +1664,30 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
test_path_is_dir patchset
'
+test_expect_success 'format-patch format.outputDirectoryCmd option' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ rm -fr patches &&
+ git format-patch master..side &&
+ test $(git rev-list master..side | wc -l) -eq $(ls patches | wc -l)
+'
+
+test_expect_success 'format-patch format.outputDirectory overrides format.outputDirectoryCmd' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ test_config format.outputDirectory patchset &&
+ rm -fr patches patchset &&
+ git format-patch master..side &&
+ test_path_is_missing patches &&
+ test_path_is_dir patchset
+'
+
+test_expect_success 'format-patch -o overrides format.outputDirectoryCmd' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ rm -fr patches patchset &&
+ git format-patch -o patchset master..side &&
+ test_path_is_missing patches &&
+ test_path_is_dir patchset
+'
+
test_expect_success 'format-patch --base' '
git checkout patchid &&
git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&
--
2.23.0.11.g242cf7f110
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC PATCH 3/3] format-patch: use a command to generate the output directory name
2019-10-02 21:26 ` [RFC PATCH 3/3] format-patch: use a command to generate the output directory name Bert Wesarg
@ 2019-10-03 0:57 ` Junio C Hamano
2019-10-05 8:43 ` [PATCH v2 1/2] format-patch: create leading components of output directory Bert Wesarg
1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2019-10-03 0:57 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git, Alexander Kuleshov, Eric Sunshine
Bert Wesarg <bert.wesarg@googlemail.com> writes:
> Having 'format.outputDirectory' is convenient, but being able to process
> all produced patches via a wildcard command is even more so. I.e.,
> using an argument like '<dir>/*'. Neither '-o' nor
> 'format.outputDirectory' can be parameterized to produce a new unique
> directory. Thus provide the new 'format.outputDirectoryCmd' configuration
> to specify a command which does the job and puts the name to standard
> output.
>
The above does not interest me (a reader) enough, without the log
message explaining why
git format-patch -o "$(outdircmd)"
is not sufficient.
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] format-patch: create leading components of output directory
@ 2019-10-05 8:43 ` Bert Wesarg
2019-10-05 8:43 ` [PATCH v2 2/2] [RFC] format-patch: configure a command to generate the output directory name Bert Wesarg
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Bert Wesarg @ 2019-10-05 8:43 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Denton Liu
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(-)
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 &&
--
2.23.0.11.g242cf7f110
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/2] [RFC] format-patch: configure a command to generate the output directory name
2019-10-05 8:43 ` [PATCH v2 1/2] format-patch: create leading components of output directory Bert Wesarg
@ 2019-10-05 8:43 ` Bert Wesarg
2019-10-06 0:57 ` [PATCH v2 1/2] format-patch: create leading components of output directory Junio C Hamano
2019-10-07 21:03 ` SZEDER Gábor
2 siblings, 0 replies; 14+ messages in thread
From: Bert Wesarg @ 2019-10-05 8:43 UTC (permalink / raw)
To: git
Cc: Bert Wesarg, Alexander Kuleshov, Eric Sunshine, Denton Liu,
Junio C Hamano
The 'format.outputDirectory' configuration is only able to store constant
directory names. Though some may use
$ git format-patch -o $(createdir) …
to name the directory dynamically. Provide a new configuration to be able
to store such a command too.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
ChangeLog:
v2:
* rephrase motivation
Cc: Alexander Kuleshov <kuleshovmail@gmail.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>
Cc: Denton Liu <liu.denton@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
---
Documentation/config/format.txt | 5 +++++
Documentation/git-format-patch.txt | 6 +++++-
builtin/log.c | 24 +++++++++++++++++++++++-
t/t4014-format-patch.sh | 25 +++++++++++++++++++++++++
4 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 40cad9278f..420188a1c6 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -83,6 +83,11 @@ format.outputDirectory::
Set a custom directory to store the resulting files instead of the
current working directory. All directory components will be created.
+format.outputDirectoryCmd::
+ The command which is used to name a custom directory to store the
+ resulting files instead of the current working directory. All directory
+ components will be created.
+
format.useAutoBase::
A boolean value which lets you enable the `--base=auto` option of
format-patch by default.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 2035d4d5d5..4936b9f91d 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -67,7 +67,11 @@ 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 .`. All directory
-components will be created.
+components will be created. The 'format.outputDirectoryCmd' configuration can
+be used to name a command to produce the directory name programmatically. The
+command should produce the name to its standard output. The
+`format.outputDirectory` configuration takes precedence over
+`format.outputDirectoryCmd`.
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 294534aacb..e841306e61 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -775,6 +775,7 @@ static const char *signature = git_version_string;
static const char *signature_file;
static int config_cover_letter;
static const char *config_output_directory;
+static const char *config_output_directory_cmd;
enum {
COVER_UNSET,
@@ -857,6 +858,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
}
if (!strcmp(var, "format.outputdirectory"))
return git_config_string(&config_output_directory, var, value);
+ if (!strcmp(var, "format.outputdirectorycmd"))
+ return git_config_string(&config_output_directory_cmd, var, value);
if (!strcmp(var, "format.useautobase")) {
base_auto = git_config_bool(var, value);
return 0;
@@ -1757,8 +1760,27 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (rev.show_notes)
init_display_notes(&rev.notes_opt);
- if (!output_directory && !use_stdout)
+ if (!output_directory && !use_stdout) {
+ // outputDirectoryCmd can be preceeded by outputDirectory
+ if (!config_output_directory && config_output_directory_cmd) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ const char *argv[1];
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ argv[0] = config_output_directory_cmd;
+ cp.argv = argv;
+ cp.use_shell = 1;
+ rc = capture_command(&cp, &buf, PATH_MAX);
+ if (rc)
+ die(_("outputDirectoryCmd command failed: "
+ "'%s'"), config_output_directory_cmd);
+ strbuf_setlen(&buf, strcspn(buf.buf, "\r\n"));
+ config_output_directory = strbuf_detach(&buf, NULL);
+ }
+
output_directory = config_output_directory;
+ }
if (!use_stdout)
output_directory = set_outdir(prefix, output_directory);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 2f2cd6fea6..f351d26543 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1642,6 +1642,31 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
test_path_is_dir patchset
'
+test_expect_success 'format-patch format.outputDirectoryCmd option' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ rm -fr patches &&
+ git format-patch master..side &&
+ git rev-list master..side >list &&
+ test_line_count = $(ls patches | wc -l) list
+'
+
+test_expect_success 'format-patch format.outputDirectory overrides format.outputDirectoryCmd' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ test_config format.outputDirectory patchset &&
+ rm -fr patches patchset &&
+ git format-patch master..side &&
+ test_path_is_missing patches &&
+ test_path_is_dir patchset
+'
+
+test_expect_success 'format-patch -o overrides format.outputDirectoryCmd' '
+ test_config format.outputDirectoryCmd "echo patches" &&
+ rm -fr patches patchset &&
+ git format-patch -o patchset master..side &&
+ test_path_is_missing patches &&
+ test_path_is_dir patchset
+'
+
test_expect_success 'format-patch --base' '
git checkout patchid &&
--
2.23.0.11.g242cf7f110
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/2] format-patch: create leading components of output directory
2019-10-05 8:43 ` [PATCH v2 1/2] format-patch: create leading components of output directory Bert Wesarg
2019-10-05 8:43 ` [PATCH v2 2/2] [RFC] format-patch: configure a command to generate the output directory name Bert Wesarg
@ 2019-10-06 0:57 ` Junio C Hamano
2019-10-07 21:03 ` SZEDER Gábor
2 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2019-10-06 0:57 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git, Denton Liu
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/2] format-patch: create leading components of output directory
2019-10-05 8:43 ` [PATCH v2 1/2] format-patch: create leading components of output directory Bert Wesarg
2019-10-05 8:43 ` [PATCH v2 2/2] [RFC] format-patch: configure a command to generate the output directory name Bert Wesarg
2019-10-06 0:57 ` [PATCH v2 1/2] format-patch: create leading components of output directory Junio C Hamano
@ 2019-10-07 21:03 ` SZEDER Gábor
2019-10-08 3:23 ` Junio C Hamano
` (2 more replies)
2 siblings, 3 replies; 14+ messages in thread
From: SZEDER Gábor @ 2019-10-07 21:03 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git, Denton Liu
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/2] format-patch: create leading components of output directory
2019-10-07 21:03 ` SZEDER Gábor
@ 2019-10-08 3:23 ` Junio C Hamano
2019-10-08 9:06 ` Bert Wesarg
2019-10-08 9:14 ` [PATCH] t4014: treat rev-list output as the expected value Denton Liu
2 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2019-10-08 3:23 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Bert Wesarg, git, Denton Liu
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 ;-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] format-patch: create leading components of output directory
2019-10-07 21:03 ` SZEDER Gábor
2019-10-08 3:23 ` Junio C Hamano
@ 2019-10-08 9:06 ` Bert Wesarg
2019-10-08 9:14 ` [PATCH] t4014: treat rev-list output as the expected value Denton Liu
2 siblings, 0 replies; 14+ messages in thread
From: Bert Wesarg @ 2019-10-08 9:06 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git Mailing List, Denton Liu
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
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] t4014: treat rev-list output as the expected value
2019-10-07 21:03 ` SZEDER Gábor
2019-10-08 3:23 ` Junio C Hamano
2019-10-08 9:06 ` Bert Wesarg
@ 2019-10-08 9:14 ` Denton Liu
2 siblings, 0 replies; 14+ messages in thread
From: Denton Liu @ 2019-10-08 9:14 UTC (permalink / raw)
To: Git Mailing List; +Cc: Bert Wesarg, Junio C Hamano, SZEDER Gábor
In 6bd26f58ea (t4014: use test_line_count() where possible, 2019-08-27),
we converted many test cases to take advantage of the test_line_count()
function. In one conversion, we inverted the expected and actual value
as tested by test_line_count(). Although functionally correct, if
format-patch ever produced incorrect output, the debugging output would
be a bunch of hashes which would be difficult to debug.
Invert the expected and actual values provided to test_line_count() so
that if format-patch produces incorrect output, the debugging output
will be a list of human-readable files instead.
Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Thanks for point out my mistake, Szeder. This patch can be applied to
the tip of 'dl/format-patch-doc-test-cleanup' and then we should base
Bert's changes on top of this branch.
t/t4014-format-patch.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 83f52614d3..72b09896cf 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1610,8 +1610,9 @@ test_expect_success 'format-patch format.outputDirectory option' '
test_config format.outputDirectory patches &&
rm -fr patches &&
git format-patch master..side &&
- git rev-list master..side >list &&
- test_line_count = $(ls patches | wc -l) list
+ count=$(git rev-list --count master..side) &&
+ ls patches >list &&
+ test_line_count = $count list
'
test_expect_success 'format-patch -o overrides format.outputDirectory' '
--
2.23.0.248.g3a9dd8fb08
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory
2019-10-02 21:26 [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Bert Wesarg
2019-10-02 21:26 ` [PATCH 2/3] format-patch: create output directory including leading components Bert Wesarg
2019-10-02 21:26 ` [RFC PATCH 3/3] format-patch: use a command to generate the output directory name Bert Wesarg
@ 2019-10-02 21:47 ` Denton Liu
2019-10-03 16:34 ` Bert Wesarg
2 siblings, 1 reply; 14+ messages in thread
From: Denton Liu @ 2019-10-02 21:47 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
Hi Bert,
> Subject: format-patch: document and exercise that -o does only create the trailing directory
s/does only create/only creates/ ?
Anyway, as a prepatory patch, I don't think that it's necessary. Maybe
it's just me but I assume that most tools create at most one directory
deep. Even mkdir won't created nested dirs unless you pass `-p`. I
dunno.
On Wed, Oct 02, 2019 at 11:26:11PM +0200, Bert Wesarg wrote:
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
> Documentation/config/format.txt | 3 ++-
> Documentation/git-format-patch.txt | 4 +++-
> t/t4014-format-patch.sh | 16 ++++++++++++++++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
> index 414a5a8a9d..e17c5d6b0f 100644
> --- a/Documentation/config/format.txt
> +++ b/Documentation/config/format.txt
> @@ -80,7 +80,8 @@ format.coverLetter::
>
> format.outputDirectory::
> Set a custom directory to store the resulting files instead of the
> - current working directory.
> + current working directory. Only the trailing directory will be created
> + though.
>
> 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 b9b97e63ae..fe7492353e 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -66,7 +66,9 @@ 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 .`. Note that only
> +the trailing directory will be created by Git, leading directories must
> +already exists.
>
> 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/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index ca7debf1d4..bf2715a503 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1632,6 +1632,22 @@ 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 &&
> + test $(git rev-list master..side | wc -l) -eq $(ls patches | wc -l)
For test case you write, please use the following pattern:
git rev-list master..side >list &&
test_line_count = $(ls patches | wc -l) list
The first benefit is that we get to take advantage of the
test_line_count function that's already written for us. The second is
that when we write tests, we shouldn't put Git commands in the upstream
of a pipe because if they fail, their return codes will be lost and we
won't be able to fail the test properly.
> +'
> +
> +test_expect_success 'format-patch -o with leading existing directories' '
> + git format-patch -o patches/side master..side &&
> + test $(git rev-list master..side | wc -l) -eq $(ls patches/side | wc -l)
> +'
> +
> +test_expect_failure 'format-patch -o with leading non-existing directories' '
> + rm -fr patches &&
> + git format-patch -o patches/side master..side
> +'
As above, I wouldn't really call this a bug in Git. I think we should
leave this test case off until the next patch.
> +
> test_expect_success 'format-patch format.outputDirectory option' '
> test_config format.outputDirectory patches &&
> rm -fr patches &&
> --
> 2.23.0.11.g242cf7f110
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory
2019-10-02 21:47 ` [PATCH 1/3] format-patch: document and exercise that -o does only create the trailing directory Denton Liu
@ 2019-10-03 16:34 ` Bert Wesarg
0 siblings, 0 replies; 14+ messages in thread
From: Bert Wesarg @ 2019-10-03 16:34 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton,
On Wed, Oct 2, 2019 at 11:47 PM Denton Liu <liu.denton@gmail.com> wrote:
>
> Hi Bert,
>
> > Subject: format-patch: document and exercise that -o does only create the trailing directory
>
> s/does only create/only creates/ ?
>
> Anyway, as a prepatory patch, I don't think that it's necessary. Maybe
> it's just me but I assume that most tools create at most one directory
> deep. Even mkdir won't created nested dirs unless you pass `-p`. I
> dunno.
>
> On Wed, Oct 02, 2019 at 11:26:11PM +0200, Bert Wesarg wrote:
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> > Documentation/config/format.txt | 3 ++-
> > Documentation/git-format-patch.txt | 4 +++-
> > t/t4014-format-patch.sh | 16 ++++++++++++++++
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
> > index 414a5a8a9d..e17c5d6b0f 100644
> > --- a/Documentation/config/format.txt
> > +++ b/Documentation/config/format.txt
> > @@ -80,7 +80,8 @@ format.coverLetter::
> >
> > format.outputDirectory::
> > Set a custom directory to store the resulting files instead of the
> > - current working directory.
> > + current working directory. Only the trailing directory will be created
> > + though.
> >
> > 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 b9b97e63ae..fe7492353e 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -66,7 +66,9 @@ 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 .`. Note that only
> > +the trailing directory will be created by Git, leading directories must
> > +already exists.
> >
> > 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/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index ca7debf1d4..bf2715a503 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -1632,6 +1632,22 @@ 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 &&
> > + test $(git rev-list master..side | wc -l) -eq $(ls patches | wc -l)
>
> For test case you write, please use the following pattern:
>
> git rev-list master..side >list &&
> test_line_count = $(ls patches | wc -l) list
>
> The first benefit is that we get to take advantage of the
> test_line_count function that's already written for us. The second is
> that when we write tests, we shouldn't put Git commands in the upstream
> of a pipe because if they fail, their return codes will be lost and we
> won't be able to fail the test properly.
thanks. I will ensure, that this follows your
dl/format-patch-doc-test-cleanup topic.
Bert
>
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading existing directories' '
> > + git format-patch -o patches/side master..side &&
> > + test $(git rev-list master..side | wc -l) -eq $(ls patches/side | wc -l)
> > +'
> > +
> > +test_expect_failure 'format-patch -o with leading non-existing directories' '
> > + rm -fr patches &&
> > + git format-patch -o patches/side master..side
> > +'
>
> As above, I wouldn't really call this a bug in Git. I think we should
> leave this test case off until the next patch.
>
> > +
> > test_expect_success 'format-patch format.outputDirectory option' '
> > test_config format.outputDirectory patches &&
> > rm -fr patches &&
> > --
> > 2.23.0.11.g242cf7f110
> >
^ permalink raw reply [flat|nested] 14+ messages in thread