| Message ID | pull.885.v9.git.1616317225769.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v9] format-patch: allow a non-integral version numbers | expand |
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. > 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).
Eric Sunshine <sunshine@sunshineco.com> writes: > 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. Oh, we just rebuilt 'next' and anything not in 'next' is not too late to reroll. Your comments on the test part were all sensible. Thanks.
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.
> > 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. > Correct it, I still have to use `grep -v`, but I need to change the '.' to `\.` .
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 3e49bf221087..741c9f8b2b88 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -221,6 +221,11 @@ populated with placeholder text. `--subject-prefix` option) has ` v<n>` appended to it. E.g. `--reroll-count=4` may produce `v4-0001-add-makefile.patch` file that has "Subject: [PATCH v4 1/20] Add makefile" in it. + `<n>` does not have to be an integer (e.g. "--reroll-count=4.4", + or "--reroll-count=4rev2" are allowed), but the downside of + using such a reroll-count is that the range-diff/interdiff + with the previous version does not state exactly which + version the new interation is compared against. --to=<email>:: Add a `To:` header to the email headers. This is in addition diff --git a/builtin/log.c b/builtin/log.c index f67b67d80ed1..af853f111464 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1662,13 +1662,19 @@ static void print_bases(struct base_tree_info *bases, FILE *file) oidclr(&bases->base_commit); } -static const char *diff_title(struct strbuf *sb, int reroll_count, - const char *generic, const char *rerolled) +static const char *diff_title(struct strbuf *sb, + const char *reroll_count, + const char *generic, + const char *rerolled) { - if (reroll_count <= 0) + int v; + + /* RFC may be v0, so allow -v1 to diff against v0 */ + if (reroll_count && !strtol_i(reroll_count, 10, &v) && + v >= 1) + strbuf_addf(sb, rerolled, v - 1); + else strbuf_addstr(sb, generic); - else /* RFC may be v0, so allow -v1 to diff against v0 */ - strbuf_addf(sb, rerolled, reroll_count - 1); return sb->buf; } @@ -1717,7 +1723,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct strbuf buf = STRBUF_INIT; int use_patch_format = 0; int quiet = 0; - int reroll_count = -1; + const char *reroll_count = NULL; char *cover_from_description_arg = NULL; char *branch_name = NULL; char *base_commit = NULL; @@ -1751,7 +1757,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("use <sfx> instead of '.patch'")), OPT_INTEGER(0, "start-number", &start_number, N_("start numbering patches at <n> instead of 1")), - OPT_INTEGER('v', "reroll-count", &reroll_count, + OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"), N_("mark the series as Nth re-roll")), OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, N_("max length of output filename")), @@ -1862,9 +1868,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (cover_from_description_arg) cover_from_description_mode = parse_cover_from_description(cover_from_description_arg); - if (0 < reroll_count) { + if (reroll_count) { struct strbuf sprefix = STRBUF_INIT; - strbuf_addf(&sprefix, "%s v%d", + + strbuf_addf(&sprefix, "%s v%s", rev.subject_prefix, reroll_count); rev.reroll_count = reroll_count; rev.subject_prefix = strbuf_detach(&sprefix, NULL); diff --git a/log-tree.c b/log-tree.c index 4531cebfab38..ebdeb29f92a2 100644 --- a/log-tree.c +++ b/log-tree.c @@ -368,9 +368,14 @@ void fmt_output_subject(struct strbuf *filename, int nr = info->nr; int start_len = filename->len; int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1); + struct strbuf temp = STRBUF_INIT; - if (0 < info->reroll_count) - strbuf_addf(filename, "v%d-", info->reroll_count); + 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); + } strbuf_addf(filename, "%04d-%s", nr, subject); if (max_len < filename->len) diff --git a/revision.h b/revision.h index e6be3c845e66..097d08354c61 100644 --- a/revision.h +++ b/revision.h @@ -235,7 +235,7 @@ struct rev_info { const char *mime_boundary; const char *patch_suffix; int numbered_files; - int reroll_count; + const char *reroll_count; char *message_id; struct ident_split from_ident; struct string_list *ref_message_ids; diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 1b26c4c2ef91..a501949575fa 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -521,6 +521,29 @@ test_expect_success 'format-patch --range-diff as commentary' ' grep "> 1: .* new message" 0001-* ' +test_expect_success 'format-patch --range-diff reroll-count with a non-integer' ' + git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual && + test_when_finished "rm v2.9-0001-*" && + test_line_count = 1 actual && + test_i18ngrep "^Range-diff:$" v2.9-0001-* && + grep "> 1: .* new message" v2.9-0001-* +' + +test_expect_success 'format-patch --range-diff reroll-count with a integer' ' + git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual && + test_when_finished "rm v2-0001-*" && + test_line_count = 1 actual && + test_i18ngrep "^Range-diff ..* v1:$" v2-0001-* && + grep "> 1: .* new message" v2-0001-* +' + +test_expect_success 'format-patch --range-diff with v0' ' + git format-patch --range-diff=HEAD~1 -v0 HEAD~1 >actual && + test_when_finished "rm v0-0001-*" && + test_line_count = 1 actual && + test_i18ngrep "^Range-diff:$" v0-0001-* && + grep "> 1: .* new message" v0-0001-* +' test_expect_success 'range-diff overrides diff.noprefix internally' ' git -c diff.noprefix=true range-diff HEAD^... ' diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 66630c8413d5..d59a06248dbd 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -378,6 +378,22 @@ test_expect_success 'reroll count' ' ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects ' +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 +' + test_expect_success 'reroll count (-v)' ' rm -fr patches && git format-patch -o patches --cover-letter -v4 main..side >list && @@ -386,6 +402,38 @@ test_expect_success 'reroll count (-v)' ' ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects ' +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 +' + +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 +' + +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 +' + check_threading () { expect="$1" && shift && @@ -2255,6 +2303,16 @@ test_expect_success 'interdiff: reroll-count' ' test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch ' +test_expect_success 'interdiff: reroll-count with a non-integer' ' + git format-patch --cover-letter --interdiff=boop~2 -v2.2 -1 boop && + test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch +' + +test_expect_success 'interdiff: reroll-count with a integer' ' + git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop && + test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch +' + test_expect_success 'interdiff: solo-patch' ' cat >expect <<-\EOF && +fleep