| Message ID | pull.885.v8.git.1616252178414.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8] format-patch: allow a non-integral version numbers | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: As the patch text itself (assuming that the vague "compared with the previous iteration" is acceptable) is getting solid, let's nitpick the proposed log message so that we can start considering a merge to 'next'. I won't review the t/ part of the patch as I know others on CC are better at reviewing the tests than I am ;-) > From: ZheNing Hu <adlternative@gmail.com> > > Usually we can only use `format-patch -v<n>` to generate integral > version numbers patches, The "only" sounds as if you are saying that "there is no tool other than 'format-patch -v' we can use", which might be technically true, but is not what you want to stress to your readers here. You are sayig that usually people only use integral version numbers. > but sometimes a small fixup should be > 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. Also, I do not think we want to encourage such use of fractional iteration numbers. We can merely enable, without saying it is a good idea or a bad idea, leaving judgment to each project that may or may not accept a patch versioned in such a way. So avoid "should be". That makes the first part of the log message to be something like: The `-v<n>` option of `format-patch` can give nothing but an integral iteration number to patches in a series. Some people, however, prefer to mark a new iteration with only a small fixup with a non integral iteration number (e.g. an "oops, that was wrong" fix-up patch for v4 iteration may be labeled as "v4.1"). Allow `format-patch` to take such a non-integral iteration number. > `<n>` can be any string, such as '3.1' or '4rev2'. In the case > where it is a non-integral value, the "Range-diff" and "Interdiff" > headers will not include the previous version. This part is well written and can stay as-is. > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > [GSOC] format-patch: allow a non-integral version numbers > > 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? Didn't I answer this question already? > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > index 3e49bf221087..e2c29a856451 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>` may be a non-integer number. E.g. `--reroll-count=4.4` Is it on purpose that `<n>` here has an extra SP before it? > + may produce `v4.4-0001-add-makefile.patch` file that has > + "Subject: [PATCH v4.4 1/20] Add makefile" in it. > + `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch` > + file that has "Subject: [PATCH v4rev2 1/20] Add makefile" in it. I am not sure it is worth repeating the whole explanation already given to "v4" for two other. By just mentioning that the "v4" could be "v4.4" or "v4vis", the readers are intelligent enough to infer what you mean, I would think. Also be honest to readers by telling them what they lose if they use a non-integral reroll count. `<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. or something like that, perhaps? Thanks.
Junio C Hamano <gitster@pobox.com> 于2021年3月21日周日 上午3:55写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > As the patch text itself (assuming that the vague "compared with the > previous iteration" is acceptable) is getting solid, let's nitpick > the proposed log message so that we can start considering a merge to > 'next'. I also think that this patch series has been on the mailing list for a long time. > > I won't review the t/ part of the patch as I know others on CC are > better at reviewing the tests than I am ;-) > Maybe I should look at Eric Sunshine's opinion :) > > > From: ZheNing Hu <adlternative@gmail.com> > > > > Usually we can only use `format-patch -v<n>` to generate integral > > version numbers patches, > > The "only" sounds as if you are saying that "there is no tool other > than 'format-patch -v' we can use", which might be technically true, > but is not what you want to stress to your readers here. You are > sayig that usually people only use integral version numbers. > You are right, this may be another place where I am not clear. "only" should be refers to "intergral version", not "format-patch". > > but sometimes a small fixup should be > > 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. > > Also, I do not think we want to encourage such use of fractional > iteration numbers. We can merely enable, without saying it is a > good idea or a bad idea, leaving judgment to each project that may > or may not accept a patch versioned in such a way. So avoid "should > be". Yes, it is more appropriate to use words similar to "can be" than "should be". > > That makes the first part of the log message to be something like: > > The `-v<n>` option of `format-patch` can give nothing but an > integral iteration number to patches in a series. Some people, > however, prefer to mark a new iteration with only a small fixup > with a non integral iteration number (e.g. an "oops, that was > wrong" fix-up patch for v4 iteration may be labeled as "v4.1"). > > Allow `format-patch` to take such a non-integral iteration > number. > > > `<n>` can be any string, such as '3.1' or '4rev2'. In the case > > where it is a non-integral value, the "Range-diff" and "Interdiff" > > headers will not include the previous version. > > This part is well written and can stay as-is. > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > [GSOC] format-patch: allow a non-integral version numbers > > > > 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? > > Didn't I answer this question already? > Sorry, forgot to update GGG cover-letter messages. > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > > index 3e49bf221087..e2c29a856451 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>` may be a non-integer number. E.g. `--reroll-count=4.4` > > Is it on purpose that `<n>` here has an extra SP before it? > > > + may produce `v4.4-0001-add-makefile.patch` file that has > > + "Subject: [PATCH v4.4 1/20] Add makefile" in it. > > + `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch` > > + file that has "Subject: [PATCH v4rev2 1/20] Add makefile" in it. > > I am not sure it is worth repeating the whole explanation already > given to "v4" for two other. By just mentioning that the "v4" could > be "v4.4" or "v4vis", the readers are intelligent enough to infer > what you mean, I would think. Also be honest to readers by telling > them what they lose if they use a non-integral reroll count. > > `<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. > > or something like that, perhaps? > This way of writing is much more concise than what I did before. I need to exercise my ability to write documents :p > Thanks. Thanks, Junio.
On Sat, Mar 20, 2021 at 10:56 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > @@ -221,6 +221,11 @@ populated with placeholder text. > + `<n>` may be a non-integer number. E.g. `--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. > + `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch` > + file that has "Subject: [PATCH v4rev2 1/20] Add makefile" in it. This new example raises the question about what happens if the argument to --reroll-count contains characters which don't belong in pathnames. For instance, what happens if `--reroll-count=1/2` is specified? Most likely, it will fail trying to write the "v1/2-whatever.patch" file to a nonexistent directory named "v1". > diff --git a/log-tree.c b/log-tree.c > @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename, > + if (info->reroll_count) > + strbuf_addf(filename, "v%s-", info->reroll_count); > strbuf_addf(filename, "%04d-%s", nr, subject); To protect against that problem, you may need to call format_sanitized_subject() manually after formatting "v%s-". (I'm just looking at this code for the first time, so I could be hopelessly wrong. There may be a better way to fix it.)
Eric Sunshine <sunshine@sunshineco.com> writes: > This new example raises the question about what happens if the > argument to --reroll-count contains characters which don't belong in > pathnames. For instance, what happens if `--reroll-count=1/2` is > specified? Most likely, it will fail trying to write the > "v1/2-whatever.patch" file to a nonexistent directory named "v1". > >> diff --git a/log-tree.c b/log-tree.c >> @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename, >> + if (info->reroll_count) >> + strbuf_addf(filename, "v%s-", info->reroll_count); >> strbuf_addf(filename, "%04d-%s", nr, subject); > > To protect against that problem, you may need to call > format_sanitized_subject() manually after formatting "v%s-". (I'm just > looking at this code for the first time, so I could be hopelessly > wrong. There may be a better way to fix it.) This kind of discovery of what I and others missed is why I love to see reviews on this list ;-) Yes, slash is of course very problematic, but what we've been doing to the patch filenames was to ensure that they will be free of $IFS whitespaces and shell glob special characters as well, and we should treat the "reroll count" just like the other end-user controlled input, i.e. the title of the patch, and sanitize it the same way. So I am pretty sure format_sanitized_subject() is the right way to go.
On Sun, Mar 21, 2021 at 1:45 AM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > To protect against that problem, you may need to call > > format_sanitized_subject() manually after formatting "v%s-". (I'm just > > looking at this code for the first time, so I could be hopelessly > > wrong. There may be a better way to fix it.) > > Yes, slash is of course very problematic, but what we've been doing > to the patch filenames was to ensure that they will be free of $IFS > whitespaces and shell glob special characters as well, and we should > treat the "reroll count" just like the other end-user controlled > input, i.e. the title of the patch, and sanitize it the same way. > > So I am pretty sure format_sanitized_subject() is the right way to > go. The pathname sanitization would also deserve a test. Denton's seemingly simple feature request[1] has turned out to be quite a little project. [1]: https://github.com/gitgitgadget/git/issues/882
Eric Sunshine <sunshine@sunshineco.com> 于2021年3月21日周日 下午12:05写道： > > On Sat, Mar 20, 2021 at 10:56 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > > @@ -221,6 +221,11 @@ populated with placeholder text. > > + `<n>` may be a non-integer number. E.g. `--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. > > + `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch` > > + file that has "Subject: [PATCH v4rev2 1/20] Add makefile" in it. > > This new example raises the question about what happens if the > argument to --reroll-count contains characters which don't belong in > pathnames. For instance, what happens if `--reroll-count=1/2` is > specified? Most likely, it will fail trying to write the > "v1/2-whatever.patch" file to a nonexistent directory named "v1". > > > diff --git a/log-tree.c b/log-tree.c > > @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename, > > + if (info->reroll_count) > > + strbuf_addf(filename, "v%s-", info->reroll_count); > > strbuf_addf(filename, "%04d-%s", nr, subject); > > To protect against that problem, you may need to call > format_sanitized_subject() manually after formatting "v%s-". (I'm just > looking at this code for the first time, so I could be hopelessly > wrong. There may be a better way to fix it.) Hi, Eric, This is a kind of "injection" problem, thank you for your discovery and solution method.
On Sun, Mar 21, 2021 at 01:54:15AM -0400, Eric Sunshine wrote: > On Sun, Mar 21, 2021 at 1:45 AM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > To protect against that problem, you may need to call > > > format_sanitized_subject() manually after formatting "v%s-". (I'm just > > > looking at this code for the first time, so I could be hopelessly > > > wrong. There may be a better way to fix it.) > > > > Yes, slash is of course very problematic, but what we've been doing > > to the patch filenames was to ensure that they will be free of $IFS > > whitespaces and shell glob special characters as well, and we should > > treat the "reroll count" just like the other end-user controlled > > input, i.e. the title of the patch, and sanitize it the same way. > > > > So I am pretty sure format_sanitized_subject() is the right way to > > go. > > The pathname sanitization would also deserve a test. > > Denton's seemingly simple feature request[1] has turned out to be > quite a little project. Sorry I've been quite busy the past couple of weeks so I haven't had the bandwidth to review the patches as they've come up. Thanks for implementing my feature request, ZheNing. And thanks for the careful reviews, Eric. > [1]: https://github.com/gitgitgadget/git/issues/882
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 3e49bf221087..e2c29a856451 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>` may be a non-integer number. E.g. `--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. + `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch` + file that has "Subject: [PATCH v4rev2 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..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..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..457312793d7e 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,22 @@ 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 --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 +' + check_threading () { expect="$1" && shift && @@ -2255,6 +2287,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