| Message ID | pull.885.git.1614269753194.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | format-patch: allow a non-integral version numbers | expand |
On Thu, Feb 25, 2021 at 11:19 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > Usually we can only use `format-patch -v<n>` to generate integral > version numbers patches, but if we can provide `format-patch` with > non-integer versions numbers of patches, this may help us to send patches > such as "v1.1" versions sometimes. On the Git project itself, fractional or non-numeric re-roll "numbers" are not necessarily encouraged[1], so this feature may not be particularly useful here, though perhaps some other project might benefit from it(?). Usually, you would want to justify why the change is desirable. Denton did give a bit of justification in his proposal[2] for this feature, so perhaps update this commit message by copying some of what he wrote as justification. [1]: I think I've only seen Denton send fractional re-rolls; other people sometimes send a periodic "fixup!" patch, but both approaches place extra burden on the project maintainer than merely re-rolling the entire series with a new integer re-roll count. [2]: https://github.com/gitgitgadget/git/issues/882 > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > @@ -215,12 +215,12 @@ populated with placeholder text. > -v <n>:: > --reroll-count=<n>:: > - Mark the series as the <n>-th iteration of the topic. The > + Mark the series as the specified version of the topic. The > output filenames have `v<n>` prepended to them, and the > subject prefix ("PATCH" by default, but configurable via the > `--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. > + `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch` > + file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it. I'm not sure we want to encourage the use of fractional re-roll counts by using it in an example like this. It would probably be better to leave the example as-is. If you really want people to know that fractional re-roll counts are supported, perhaps add separate sentence saying that they are. > diff --git a/builtin/log.c b/builtin/log.c > @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file) > -static const char *diff_title(struct strbuf *sb, int reroll_count, > +static const char *diff_title(struct strbuf *sb, const char *reroll_count, > const char *generic, const char *rerolled) > { > - if (reroll_count <= 0) > + if (!reroll_count) > strbuf_addstr(sb, generic); > else /* RFC may be v0, so allow -v1 to diff against v0 */ > - strbuf_addf(sb, rerolled, reroll_count - 1); > + strbuf_addf(sb, rerolled, "last version"); > return sb->buf; > } There are a couple problems here (at least). First, the string "last version" should be localizable, `_("last version")`. Second, in Denton's proposal[2], he suggested using the string "last version" _only_ if the re-roll count is not an integer. What you have here applies "last version" unconditionally when -v is used so that the outcome is _always_ "Range-diff since last version". If that's what you intend to do, there's no reason to do any sort of interpolation using the template "Range-diff since %". What Denton had in mind was this (using pseudo-code): if re-roll count not specified: message = "Range-diff" else if re-roll count is integer: message = "Range-diff since v%d", re-roll else: message = "Range-diff since v%s", re-roll However, there isn't a good reason to favor "Range-diff since last version" over the simpler generic message "Range-diff". So, the above should be collapsed to: if re-roll count is specified and integer: message = "Range-diff since v%d", re-roll else: message = "Range-diff" > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > - _("Interdiff against v%d:")); > + _("Interdiff against %s:")); > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > - _("Range-diff against v%d:")); > + _("Range-diff against %s:")); If you follow my recommendation above using the simplified conditional, then you don't need to drop the "v" since you won't be saying "last version".
"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 if we can provide `format-patch` with > non-integer versions numbers of patches, this may help us to send patches > such as "v1.1" versions sometimes. A pro-tip. Do not make your change sound like "because we can do this"; a change sells a lot easier if it were "because we need this---see how useful it is in such and such cases". I am not in principle opposed to support a fractional version number that comes after an integral one, which allows users to signal that it is a minor correction that does not deserve a full version bump. But not in this form. - This implementation regresses the output of the diff_version() helper function, and does so deliberately, even by butchering a test that protects it to hide the regression from the test suite. "This is the change from the last version" is expecting too much from the reviewers. Those who may recall seeing your v3 may have been otherwise occupied for a few weeks and ununware of your v4. - It also closes the door to a feature that is occasionally requested to update the make_cover_letter() to carry title and description over from the previous iteration. If we were to do this, I would probably suggest a preliminary patch that refactors the hardcoded "reroll_count - 1" out of diff_title() so that the helper takes two "reroll count strings", i.e. reroll count for this round, and the previous round, as two separate parameters. Teach the caller to pass "reroll_count - 1" for the new parameter in this preliminary step. Then in the main patch, soon after we finish parsing the command line options to learn we got "-v$N" argument: (0) It might be acceptable to teach the command a new option to allow end-users to explicitly give the previous reroll count string from the command line. If we were to do so, skip everything below when such an option is used and use the user-supplied one. (1) If $N is an integer, use $N and $N-1 as the reroll count strings for this and previous round. (2) Otherwise, scan for "v(.*)-0*-cover-letter.$suffix" in the output directory to find all previous rerolls, and pick the one that sorts the last and before $N (use versioncmp). If there are existing v1-, v2-, etc., and if we are given "-v2.1" from the command line, we'd want to grab v2 as the previous reroll count. (3) After doing the above steps, if we still do not have the previous reroll count string, error out. You could argue that we may optionally work in degraded mode, using "last version" as the fallback value for the previous round count string, and it may work for some things (e.g. the "change against the last round" label) and not for other things (e.g. reuse description in the previous cover letter). I would not recommend it. And then, we have two reroll count strings that we can feed diff_title(); we also can later use the reroll count string for the previous round to teach make_cover_letter() read and recover the description from the cover letter of the previous round. Thanks. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > [GSOC] format-patch: allow a non-integral version numbers > > * format-patch previously only integer version number -v<n>, now trying > to provide a non-integer version. > > this want to fix #882 Thanks. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/885 > > Documentation/git-format-patch.txt | 6 +++--- > builtin/log.c | 20 ++++++++++---------- > log-tree.c | 4 ++-- > revision.h | 2 +- > t/t4014-format-patch.sh | 8 ++++---- > 5 files changed, 20 insertions(+), 20 deletions(-) > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > index 3e49bf221087..0cc39dbf573c 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -215,12 +215,12 @@ populated with placeholder text. > > -v <n>:: > --reroll-count=<n>:: > - Mark the series as the <n>-th iteration of the topic. The > + Mark the series as the specified version of the topic. The > output filenames have `v<n>` prepended to them, and the > subject prefix ("PATCH" by default, but configurable via the > `--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. > + `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch` > + file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it. > > --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..72af140b812e 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1662,13 +1662,13 @@ 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, > +static const char *diff_title(struct strbuf *sb, const char *reroll_count, > const char *generic, const char *rerolled) > { > - if (reroll_count <= 0) > + if (!reroll_count) > strbuf_addstr(sb, generic); > else /* RFC may be v0, so allow -v1 to diff against v0 */ > - strbuf_addf(sb, rerolled, reroll_count - 1); > + strbuf_addf(sb, rerolled, "last version"); > return sb->buf; > } > > @@ -1717,7 +1717,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,8 +1751,8 @@ 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, > - N_("mark the series as Nth re-roll")), > + OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"), > + N_("mark the series as specified version re-roll")), > OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, > N_("max length of output filename")), > OPT_CALLBACK_F(0, "rfc", &rev, NULL, > @@ -1862,9 +1862,9 @@ 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); > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > rev.idiff_oid2 = get_commit_tree_oid(list[0]); > rev.idiff_title = diff_title(&idiff_title, reroll_count, > _("Interdiff:"), > - _("Interdiff against v%d:")); > + _("Interdiff against %s:")); > } > > if (creation_factor < 0) > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > rev.creation_factor = creation_factor; > rev.rdiff_title = diff_title(&rdiff_title, reroll_count, > _("Range-diff:"), > - _("Range-diff against v%d:")); > + _("Range-diff against %s:")); > } > > if (!signature) { > 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/t4014-format-patch.sh b/t/t4014-format-patch.sh > index 66630c8413d5..b911e08f0810 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -372,10 +372,10 @@ test_expect_success 'filename limit applies only to basename' ' > > test_expect_success 'reroll count' ' > rm -fr patches && > - git format-patch -o patches --cover-letter --reroll-count 4 main..side >list && > - ! grep -v "^patches/v4-000[0-3]-" list && > + 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 [0-3]/3\] " subjects > + ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects > ' > > test_expect_success 'reroll count (-v)' ' > @@ -2252,7 +2252,7 @@ test_expect_success 'interdiff: cover-letter' ' > > test_expect_success 'interdiff: reroll-count' ' > git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop && > - test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch > + test_i18ngrep "^Interdiff ..* last version:$" v2-0000-cover-letter.patch > ' > > test_expect_success 'interdiff: solo-patch' ' > > base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07
Eric Sunshine <sunshine@sunshineco.com> 于2021年2月26日周五 上午1:57写道： > > On Thu, Feb 25, 2021 at 11:19 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > Usually we can only use `format-patch -v<n>` to generate integral > > version numbers patches, but if we can provide `format-patch` with > > non-integer versions numbers of patches, this may help us to send patches > > such as "v1.1" versions sometimes. > > On the Git project itself, fractional or non-numeric re-roll "numbers" > are not necessarily encouraged[1], so this feature may not be > particularly useful here, though perhaps some other project might > benefit from it(?). Usually, you would want to justify why the change > is desirable. Denton did give a bit of justification in his > proposal[2] for this feature, so perhaps update this commit message by > copying some of what he wrote as justification. > OK, I will remember it. > [1]: I think I've only seen Denton send fractional re-rolls; other > people sometimes send a periodic "fixup!" patch, but both approaches > place extra burden on the project maintainer than merely re-rolling > the entire series with a new integer re-roll count. > > [2]: https://github.com/gitgitgadget/git/issues/882 > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > > @@ -215,12 +215,12 @@ populated with placeholder text. > > -v <n>:: > > --reroll-count=<n>:: > > - Mark the series as the <n>-th iteration of the topic. The > > + Mark the series as the specified version of the topic. The > > output filenames have `v<n>` prepended to them, and the > > subject prefix ("PATCH" by default, but configurable via the > > `--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. > > + `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch` > > + file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it. > > I'm not sure we want to encourage the use of fractional re-roll counts > by using it in an example like this. It would probably be better to > leave the example as-is. If you really want people to know that > fractional re-roll counts are supported, perhaps add separate sentence > saying that they are. Yes, but the original description `<n>-th iteration` may imply that the version number is an integer. Is there any good way to solve it? > > > diff --git a/builtin/log.c b/builtin/log.c > > @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file) > > -static const char *diff_title(struct strbuf *sb, int reroll_count, > > +static const char *diff_title(struct strbuf *sb, const char *reroll_count, > > const char *generic, const char *rerolled) > > { > > - if (reroll_count <= 0) > > + if (!reroll_count) > > strbuf_addstr(sb, generic); > > else /* RFC may be v0, so allow -v1 to diff against v0 */ > > - strbuf_addf(sb, rerolled, reroll_count - 1); > > + strbuf_addf(sb, rerolled, "last version"); > > return sb->buf; > > } > > There are a couple problems here (at least). First, the string "last > version" should be localizable, `_("last version")`. Second, in > Denton's proposal[2], he suggested using the string "last version" > _only_ if the re-roll count is not an integer. What you have here > applies "last version" unconditionally when -v is used so that the > outcome is _always_ "Range-diff since last version". If that's what > you intend to do, there's no reason to do any sort of interpolation > using the template "Range-diff since %". What Denton had in mind was > this (using pseudo-code): > > if re-roll count not specified: > message = "Range-diff" > else if re-roll count is integer: > message = "Range-diff since v%d", re-roll > else: > message = "Range-diff since v%s", re-roll > > However, there isn't a good reason to favor "Range-diff since last > version" over the simpler generic message "Range-diff". So, the above > should be collapsed to:interpolation > > if re-roll count is specified and integer: > message = "Range-diff since v%d", re-roll > else: > message = "Range-diff" > You mean using "Range-diff since %" may not be as good as" Range-diff" without sorting. I agree with you. > > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > - _("Interdiff against v%d:")); > > + _("Interdiff against %s:")); > > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > - _("Range-diff against v%d:")); > > + _("Range-diff against %s:")); > > If you follow my recommendation above using the simplified > conditional, then you don't need to drop the "v" since you won't be > saying "last version". Your suggestion is very good and easy to implement, but I may have made some changes in Junio suggestion later, that is, I used the `previous_count` method to provide it to `diff_title()`. I will explain my thoughts and problems in my reply to Junio. You'll see it later. Thank you for your help. -- ZheNing Hu
Junio C Hamano <gitster@pobox.com> 于2021年2月26日周五 上午2:13写道： > > "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 if we can provide `format-patch` with > > non-integer versions numbers of patches, this may help us to send patches > > such as "v1.1" versions sometimes. > > A pro-tip. Do not make your change sound like "because we can do > this"; a change sells a lot easier if it were "because we need > this---see how useful it is in such and such cases". OK. I will fully express the advantages of these patches in the future. > > I am not in principle opposed to support a fractional version number > that comes after an integral one, which allows users to signal that > it is a minor correction that does not deserve a full version bump. > > But not in this form. > > - This implementation regresses the output of the diff_version() > helper function, and does so deliberately, even by butchering a > test that protects it to hide the regression from the test suite. > > "This is the change from the last version" is expecting too much > from the reviewers. Those who may recall seeing your v3 may have > been otherwise occupied for a few weeks and ununware of your v4. > > - It also closes the door to a feature that is occasionally > requested to update the make_cover_letter() to carry title and > description over from the previous iteration. > Indeed. > If we were to do this, I would probably suggest a preliminary patch > that refactors the hardcoded "reroll_count - 1" out of diff_title() > so that the helper takes two "reroll count strings", i.e. reroll > count for this round, and the previous round, as two separate > parameters. Teach the caller to pass "reroll_count - 1" for the new > parameter in this preliminary step. > I like the idea. > Then in the main patch, soon after we finish parsing the command > line options to learn we got "-v$N" argument: > > (0) It might be acceptable to teach the command a new option to > allow end-users to explicitly give the previous reroll count > string from the command line. If we were to do so, skip > everything below when such an option is used and use the > user-supplied one. > > (1) If $N is an integer, use $N and $N-1 as the reroll count > strings for this and previous round. > My patch still in WIP state,but (1) work here. > (2) Otherwise, scan for "v(.*)-0*-cover-letter.$suffix" in the > output directory to find all previous rerolls, and pick the one > that sorts the last and before $N (use versioncmp). If there > are existing v1-, v2-, etc., and if we are given "-v2.1" from > the command line, we'd want to grab v2 as the previous reroll > count. > I have some doubts: (1) if the user generates multiple patches through the `git format patch -v<n>`, they may be placed under different folders, we may not assert what the version number of the previous version of the patch is by looking at all patches in one directory. (2) "0000-1234-cover-letter.patch" this form may also need to be considered in the v(. *)-0*- cover-letter.$suffix". for the time being, this approach will be more cumbersome. > (3) After doing the above steps, if we still do not have the > previous reroll count string, error out. > > You could argue that we may optionally work in degraded mode, > using "last version" as the fallback value for the previous > round count string, and it may work for some things (e.g. the > "change against the last round" label) and not for other things > (e.g. reuse description in the previous cover letter). I would > not recommend it. > > And then, we have two reroll count strings that we can feed > diff_title(); we also can later use the reroll count string for the > previous round to teach make_cover_letter() read and recover the > description from the cover letter of the previous round. > > Thanks. > I think the user should be allowed to provide the previous version number in the console. If the specified `reroll_count=<n>` is integrated, then we will use `n-1` in the `diff_title`, or if the user specifies ` previous_count `, we will use `previous_count` in `diff title`, otherwise, `generic` in the parameter of `diff_title` will be used. I don’t know if my ideas like this are inadequate, hope you can point it out. > > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > [GSOC] format-patch: allow a non-integral version numbers > > > > * format-patch previously only integer version number -v<n>, now trying > > to provide a non-integer version. > > > > this want to fix #882 Thanks. > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/885 > > > > Documentation/git-format-patch.txt | 6 +++--- > > builtin/log.c | 20 ++++++++++---------- > > log-tree.c | 4 ++-- > > revision.h | 2 +- > > t/t4014-format-patch.sh | 8 ++++---- > > 5 files changed, 20 insertions(+), 20 deletions(-) > > > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > > index 3e49bf221087..0cc39dbf573c 100644 > > --- a/Documentation/git-format-patch.txt > > +++ b/Documentation/git-format-patch.txt > > @@ -215,12 +215,12 @@ populated with placeholder text. > > > > -v <n>:: > > --reroll-count=<n>:: > > - Mark the series as the <n>-th iteration of the topic. The > > + Mark the series as the specified version of the topic. The > > output filenames have `v<n>` prepended to them, and the > > subject prefix ("PATCH" by default, but configurable via the > > `--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. > > + `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch` > > + file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it. > > > > --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..72af140b812e 100644 > > --- a/builtin/log.c > > +++ b/builtin/log.c > > @@ -1662,13 +1662,13 @@ 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, > > +static const char *diff_title(struct strbuf *sb, const char *reroll_count, > > const char *generic, const char *rerolled) > > { > > - if (reroll_count <= 0) > > + if (!reroll_count) > > strbuf_addstr(sb, generic); > > else /* RFC may be v0, so allow -v1 to diff against v0 */ > > - strbuf_addf(sb, rerolled, reroll_count - 1); > > + strbuf_addf(sb, rerolled, "last version"); > > return sb->buf; > > } > > > > @@ -1717,7 +1717,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,8 +1751,8 @@ 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, > > - N_("mark the series as Nth re-roll")), > > + OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"), > > + N_("mark the series as specified version re-roll")), > > OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, > > N_("max length of output filename")), > > OPT_CALLBACK_F(0, "rfc", &rev, NULL, > > @@ -1862,9 +1862,9 @@ 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); > > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > rev.idiff_oid2 = get_commit_tree_oid(list[0]); > > rev.idiff_title = diff_title(&idiff_title, reroll_count, > > _("Interdiff:"), > > - _("Interdiff against v%d:")); > > + _("Interdiff against %s:")); > > } > > > > if (creation_factor < 0) > > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > rev.creation_factor = creation_factor; > > rev.rdiff_title = diff_title(&rdiff_title, reroll_count, > > _("Range-diff:"), > > - _("Range-diff against v%d:")); > > + _("Range-diff against %s:")); > > } > > > > if (!signature) { > > 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/t4014-format-patch.sh b/t/t4014-format-patch.sh > > index 66630c8413d5..b911e08f0810 100755 > > --- a/t/t4014-format-patch.sh > > +++ b/t/t4014-format-patch.sh > > @@ -372,10 +372,10 @@ test_expect_success 'filename limit applies only to basename' ' > > > > test_expect_success 'reroll count' ' > > rm -fr patches && > > - git format-patch -o patches --cover-letter --reroll-count 4 main..side >list && > > - ! grep -v "^patches/v4-000[0-3]-" list && > > + 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 [0-3]/3\] " subjects > > + ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects > > ' > > > > test_expect_success 'reroll count (-v)' ' > > @@ -2252,7 +2252,7 @@ test_expect_success 'interdiff: cover-letter' ' > > > > test_expect_success 'interdiff: reroll-count' ' > > git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop && > > - test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch > > + test_i18ngrep "^Interdiff ..* last version:$" v2-0000-cover-letter.patch > > ' > > > > test_expect_success 'interdiff: solo-patch' ' > > > > base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07
On 2021-02-25 10:13:24-0800, Junio C Hamano <gitster@pobox.com> wrote: > "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 if we can provide `format-patch` with > > non-integer versions numbers of patches, this may help us to send patches > > such as "v1.1" versions sometimes. > > I am not in principle opposed to support a fractional version number > that comes after an integral one, which allows users to signal that > it is a minor correction that does not deserve a full version bump. I'm late into the party, but this rational sounds greate to me. > If we were to do this, I would probably suggest a preliminary patch > that refactors the hardcoded "reroll_count - 1" out of diff_title() > so that the helper takes two "reroll count strings", i.e. reroll > count for this round, and the previous round, as two separate > parameters. Teach the caller to pass "reroll_count - 1" for the new > parameter in this preliminary step. However, if it's only a minor correction to the major version, I _think_ it makes better sense to compare with the major version instead of comparing with another minor version. When a reviewer reviews v3.5, they can just compare to v3. In a hypothetical world, when another reviewer jump in and a major change required, v4 reroll also compare with v3. In other words, we will have something likes: - v3 vs v2 - v3.1 vs v3 - v3.2 vs v3 .... - v4 vs v3 The good side of this approach is: the logic to choose previous version is simple. The downside of this approach is: reviewers need to re-read the changes in v3.1 v3.2, etc... However, we can reasonably expect those changes are small enough, they're minor changes after all. And they will need to re-read all the change if the major verison was increased.
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> If we were to do this, I would probably suggest a preliminary patch >> that refactors the hardcoded "reroll_count - 1" out of diff_title() >> so that the helper takes two "reroll count strings", i.e. reroll >> count for this round, and the previous round, as two separate >> parameters. Teach the caller to pass "reroll_count - 1" for the new >> parameter in this preliminary step. > > However, if it's only a minor correction to the major version, > I _think_ it makes better sense to compare with the major version > instead of comparing with another minor version. I wanted to have no opinion on this, as what is expected out of the fractional iteration count by people would be different depending on whom you ask. The "-1" suggestion was a fallback to allow those who supply integral reroll count not to explicitly say what the previous round was from the command line. I do not particularly care how the previous round for fractional iteration count were computed by default when the user did not give one explicitly from the command line. > When a reviewer reviews v3.5, they can just compare to v3. > In a hypothetical world, when another reviewer jump in and a major > change required, v4 reroll also compare with v3. > > In other words, we will have something likes: > > - v3 vs v2 > - v3.1 vs v3 > - v3.2 vs v3 > .... > - v4 vs v3 > > The good side of this approach is: the logic to choose previous > version is simple. > > The downside of this approach is: reviewers need to re-read the > changes in v3.1 v3.2, etc... However, we can reasonably expect those > changes are small enough, they're minor changes after all. > > And they will need to re-read all the change if the major verison was > increased.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 3e49bf221087..0cc39dbf573c 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -215,12 +215,12 @@ populated with placeholder text. -v <n>:: --reroll-count=<n>:: - Mark the series as the <n>-th iteration of the topic. The + Mark the series as the specified version of the topic. The output filenames have `v<n>` prepended to them, and the subject prefix ("PATCH" by default, but configurable via the `--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. + `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch` + file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it. --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..72af140b812e 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1662,13 +1662,13 @@ 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, +static const char *diff_title(struct strbuf *sb, const char *reroll_count, const char *generic, const char *rerolled) { - if (reroll_count <= 0) + if (!reroll_count) strbuf_addstr(sb, generic); else /* RFC may be v0, so allow -v1 to diff against v0 */ - strbuf_addf(sb, rerolled, reroll_count - 1); + strbuf_addf(sb, rerolled, "last version"); return sb->buf; } @@ -1717,7 +1717,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,8 +1751,8 @@ 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, - N_("mark the series as Nth re-roll")), + OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"), + N_("mark the series as specified version re-roll")), OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, N_("max length of output filename")), OPT_CALLBACK_F(0, "rfc", &rev, NULL, @@ -1862,9 +1862,9 @@ 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); @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.idiff_oid2 = get_commit_tree_oid(list[0]); rev.idiff_title = diff_title(&idiff_title, reroll_count, _("Interdiff:"), - _("Interdiff against v%d:")); + _("Interdiff against %s:")); } if (creation_factor < 0) @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.creation_factor = creation_factor; rev.rdiff_title = diff_title(&rdiff_title, reroll_count, _("Range-diff:"), - _("Range-diff against v%d:")); + _("Range-diff against %s:")); } if (!signature) { 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/t4014-format-patch.sh b/t/t4014-format-patch.sh index 66630c8413d5..b911e08f0810 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -372,10 +372,10 @@ test_expect_success 'filename limit applies only to basename' ' test_expect_success 'reroll count' ' rm -fr patches && - git format-patch -o patches --cover-letter --reroll-count 4 main..side >list && - ! grep -v "^patches/v4-000[0-3]-" list && + 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 [0-3]/3\] " subjects + ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects ' test_expect_success 'reroll count (-v)' ' @@ -2252,7 +2252,7 @@ test_expect_success 'interdiff: cover-letter' ' test_expect_success 'interdiff: reroll-count' ' git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop && - test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch + test_i18ngrep "^Interdiff ..* last version:$" v2-0000-cover-letter.patch ' test_expect_success 'interdiff: solo-patch' '