| Message ID | pull.885.v7.git.1616152884317.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7] format-patch: allow a non-integral version numbers | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > There is a small question: in the case of --reroll-count=<n>, "n" is an > integer, we output "n-1" in the patch instead of "m" specified by > --previous-count=<m>,Should we switch the priority of these two: let "m" > output? In principle, if the end-user can explicitly give an input that may (or may not) contradict what the code would automatically compute, we should treat the latter as a fallback default and use it only when the former is not given. So, I would think the answer is "yes, we should use <m> and not <n>-1, if <m> is given".
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: ZheNing Hu <adlternative@gmail.com> > > Usually we can only use `format-patch -v<n>` to generate integral > version numbers patches, but sometimes a same fixup should be I guess that you meant "a small fixup" here? > labeled as a non-integral version like `v1.1`, so teach `format-patch` > to allow a non-integral version which may be helpful to send those > patches. > > `<n>` can be any string, such as `-v1.1`. I would probably use "such as '3.1' or '4rev2'" (without the leading "-v", as that is how you introduce <n> in the first sentence) instead in the proposed log message, and give a pair of examples to show both fractional numbers and non numbers are accepted in the documentation, if I were doing this patch. > In the case where it > is a non-integral value, the "Range-diff" and "Interdiff" > headers will not include the previous version. Which is rather unfortunate, but I do not care strongly enough (I would rather discurage use of partial rerolls with these fractinal bits anyway). > Range-diff vs v6: > > 1: d5f5e3f073de ! 1: 95cfe75ee7da format-patch: allow a non-integral version numbers > @@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file) > + int v; > + > + /* RFC may be v0, so allow -v1 to diff against v0 */ > -+ if (reroll_count && !strtol_i(reroll_count, 10, &v)) > ++ if (reroll_count && !strtol_i(reroll_count, 10, &v) && > ++ v >= 1) > + strbuf_addf(sb, rerolled, v - 1); > + else > strbuf_addstr(sb, generic); In one of your review responses earlier, you hinted that limiting the range of v like this was needed for correctness. There should be a new test to cover the case that this change makes a difference.
Junio C Hamano <gitster@pobox.com> 于2021年3月20日周六 上午1:29写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > Usually we can only use `format-patch -v<n>` to generate integral > > version numbers patches, but sometimes a same fixup should be > > I guess that you meant "a small fixup" here? > Yes. > > labeled as a non-integral version like `v1.1`, so teach `format-patch` > > to allow a non-integral version which may be helpful to send those > > patches. > > > > `<n>` can be any string, such as `-v1.1`. > > I would probably use "such as '3.1' or '4rev2'" (without the leading > "-v", as that is how you introduce <n> in the first sentence) > instead in the proposed log message, and give a pair of examples to > show both fractional numbers and non numbers are accepted in the > documentation, if I were doing this patch. > Sure, either a "fractional" version or a "non-nummbers" version can be accepted. I will complete the expansion of these documents. > > In the case where it > > is a non-integral value, the "Range-diff" and "Interdiff" > > headers will not include the previous version. > > Which is rather unfortunate, but I do not care strongly enough (I > would rather discurage use of partial rerolls with these fractinal > bits anyway). > Well, now this patch (no previous-count) was born in the previous suggestion of Denton Liu. I also hope it can be more practical. > > Range-diff vs v6: > > > > 1: d5f5e3f073de ! 1: 95cfe75ee7da format-patch: allow a non-integral version numbers > > @@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file) > > + int v; > > + > > + /* RFC may be v0, so allow -v1 to diff against v0 */ > > -+ if (reroll_count && !strtol_i(reroll_count, 10, &v)) > > ++ if (reroll_count && !strtol_i(reroll_count, 10, &v) && > > ++ v >= 1) > > + strbuf_addf(sb, rerolled, v - 1); > > + else > > strbuf_addstr(sb, generic); > > In one of your review responses earlier, you hinted that limiting > the range of v like this was needed for correctness. There should > be a new test to cover the case that this change makes a difference. I will do it. Thanks.
Junio C Hamano <gitster@pobox.com> 于2021年3月20日周六 上午12:01写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > There is a small question: in the case of --reroll-count=<n>, "n" is an > > integer, we output "n-1" in the patch instead of "m" specified by > > --previous-count=<m>,Should we switch the priority of these two: let "m" > > output? > > In principle, if the end-user can explicitly give an input that may > (or may not) contradict what the code would automatically compute, > we should treat the latter as a fallback default and use it only > when the former is not given. So, I would think the answer is "yes, > we should use <m> and not <n>-1, if <m> is given". I have always felt that this issue needs attention before, Although this `previous-count` may have nothing to do with the patch of this version, thank you for answering this doubt for me.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 3e49bf221087..82abef1a6d92 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -221,6 +221,7 @@ 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>` may be a fractional number. --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..5f2e08ebcaab 100644 --- a/log-tree.c +++ b/log-tree.c @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename, int start_len = filename->len; int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1); - if (0 < info->reroll_count) - strbuf_addf(filename, "v%d-", info->reroll_count); + if (info->reroll_count) + strbuf_addf(filename, "v%s-", info->reroll_count); 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..dc419c087e07 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -521,6 +521,22 @@ 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 '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..59dff38065ab 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -378,6 +378,14 @@ test_expect_success 'reroll count' ' ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects ' +test_expect_success 'reroll count with a non-integer' ' + 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 (-v)' ' rm -fr patches && git format-patch -o patches --cover-letter -v4 main..side >list && @@ -386,6 +394,14 @@ test_expect_success 'reroll count (-v)' ' ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects ' +test_expect_success 'reroll count (-v) with a non-integer' ' + 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 +' + check_threading () { expect="$1" && shift && @@ -2255,6 +2271,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