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

[ZheNing Hu <adlternative@gmail.com>]

Eric Sunshine <sunshine@sunshineco.com> 于2021年3月23日周二 下午1:31写道：
>
> On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > [...]
> > 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.
>

Thanks, Eric, these suggestions are worth considering.

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

Sometimes I always forget this: which variables need to be placed locally,
It seems that I have to be more careful.

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

Makes sense.

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

Yes, `grep -vF` is very important for the correctness of the test.

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

The second test I just want to test character `-`, but now I think it can
merge to first test.

Thanks.