Re: [PATCH v9] format-patch: allow a non-integral version numbers

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 23, 2021, 05:31 UTC
Message-ID: <CAPig+cQ5jBc-fz69ygpWUP7uCVNAT8GT_01y6GnxDEa6XDNZ=g@mail.gmail.com>
In-Reply-To: <pull.885.v9.git.1616317225769.gitgitgadget@gmail.com>

On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines

> [...]
> Allow `format-patch` to take such a non-integral iteration
> number.
> [...]
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

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.

Show 11 quoted lines

> 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);
        ...
    }

Show 17 quoted lines

> 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...

Show 16 quoted lines

> @@ -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.

Show 7 quoted lines

> +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`.

Show 7 quoted lines

> +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).