On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> [...]
> Allow `format-patch` to take such a non-integral iteration
> number.
> [...]
> Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx>
Just a few nits below; nothing very important (except perhaps the
final comment about the potential for people to get confused while
reading the tests). Junio already has this marked as ready to merge to
"next", so these nits may not be worth a re-roll.
> diff --git a/log-tree.c b/log-tree.c
> @@ -368,9 +368,14 @@ void fmt_output_subject(struct strbuf *filename,
> int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
> + struct strbuf temp = STRBUF_INIT;
>
> + if (info->reroll_count) {
> + strbuf_addf(&temp, "v%s", info->reroll_count);
> + format_sanitized_subject(filename, temp.buf, temp.len);
> + strbuf_addstr(filename, "-");
> + strbuf_release(&temp);
> + }
The new `temp` strbuf is use only inside the conditional, so it
could/should have been declared in that block rather than in the outer
block:
if (info->reroll_count) {
struct strbuf temp = STRBUF_INIT;
strbuf_addf(&temp, "v%s", info->reroll_count);
...
}
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -378,6 +378,22 @@ test_expect_success 'reroll count' '
> +test_expect_success 'reroll count with a fractional number' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> + ! grep -v "^patches/v4.4-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
> +test_expect_success 'reroll count with a non number' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter --reroll-count 4rev2 main..side >list &&
> + ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> +'
The above two tests...
> @@ -386,6 +402,38 @@ test_expect_success 'reroll count (-v)' '
> +test_expect_success 'reroll count (-v) with a fractional number' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter -v4.4 main..side >list &&
> + ! grep -v "^patches/v4.4-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
> +test_expect_success 'reroll (-v) count with a non number' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter -v4rev2 main..side >list &&
> + ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> +'
... are repeated here with the only difference being `--reroll-count`
versus `-v`. Since other tests have already established that
`--reroll-count` and `-v` are identical, it's not really necessary to
do that work again with these duplicate tests.
> +test_expect_success 'reroll (-v) count with a "injection (1)"' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter -v4..././../1/.2// main..side >list &&
> + ! grep -v "^patches/v4.-.-.-1-.2-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4..././../1/.2// [0-3]/3\] " subjects
> +'
A couple comments:
The test title might be easier for other people to understand if it
says "non-pathname character" or "non filename character" rather than
"injection".
Note that the `grep -v` is casting a wider net than it seems at first
glance. The `.` matches any character, not just a period ".". To
tighten the matching and make `.` match just a ".", you can use `grep
-vF`.
> +test_expect_success 'reroll (-v) count with a "injection (2)"' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter -v4-----//1//--.-- main..side >list &&
> + ! grep -v "^patches/v4-1-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4-----//1//--.-- [0-3]/3\] " subjects
> +'
Presumably the coverage of format_sanitized_subject() is already being
tested elsewhere, so it's not clear that this second "injection" test
adds any value over the first test. Moreover, this second test can
confuse readers into thinking that it is testing something that the
first test didn't cover, but that isn't the case (as far as I can
tell).
