diff mbox series

[v4] format-patch: allow a non-integral version numbers

Message ID pull.885.v4.git.1614928211635.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v4] format-patch: allow a non-integral version numbers | expand

Commit Message

ZheNing Hu March 5, 2021, 7:10 a.m. UTC 
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
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`.  In the case where it
is a non-integral value, the "Range-diff" and "Interdiff"
headers will not include the previous version.

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?
    
    this want to fix #882 Thanks.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/885

Range-diff vs v3:

 1:  d4f38b78c464 ! 1:  cb1c0267e16b format-patch: allow a non-integral version numbers
     @@ Commit message
      
          Usually we can only use `format-patch -v<n>` to generate integral
          version numbers patches, but sometimes a same fixup should be
     -    labeled as a non-integral versions like `v1.1`, so teach `format-patch`
     -    to allow a non-integral versions may be helpful to send those patches.
     +    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.
      
     -    Since the original `format-patch` logic, if we specify a version `-v<n>`
     -    and commbine with `--interdiff` or `--rangediff`, the patch will output
     -    "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
     -    not meet the requirements of our fractional version numbers, so if the
     -    user use a integral version number `-v<n>`, ensure that the output in
     -    the patch is still `v<n-1>`; otherwise, only output "Interdiff" or
     -    "Range-diff".
     +    `<n>` can be any string, such as `-v1.1`.  In the case where it
     +    is a non-integral value, the "Range-diff" and "Interdiff"
     +    headers will not include the previous version.
      
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
     @@ Documentation/git-format-patch.txt: populated with placeholder text.
       	Add a `To:` header to the email headers. This is in addition
      
       ## builtin/log.c ##
     -@@ builtin/log.c: 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_string,
     -+			const char*previous_count_string,
     -+			const char *generic, const char *rerolled)
     - {
     --	if (reroll_count <= 0)
     -+	if (!reroll_count_string || !previous_count_string)
     - 		strbuf_addstr(sb, generic);
     --	else /* RFC may be v0, so allow -v1 to diff against v0 */
     --		strbuf_addf(sb, rerolled, reroll_count - 1);
     -+	else if (previous_count_string)
     -+		strbuf_addf(sb, rerolled, previous_count_string);
     - 	return sb->buf;
     - }
     - 
      @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
       	int use_patch_format = 0;
       	int quiet = 0;
       	int reroll_count = -1;
      +	const char *reroll_count_string = NULL;
     -+	const char *previous_count_string = NULL;
       	char *cover_from_description_arg = NULL;
       	char *branch_name = NULL;
       	char *base_commit = NULL;
     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
       		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_string, N_("reroll-count"),
     -+			    N_("mark the series as specified version re-roll")),
     + 			    N_("mark the series as Nth 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,
      @@ builtin/log.c: 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);
     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      -		strbuf_addf(&sprefix, "%s v%d",
      -			    rev.subject_prefix, reroll_count);
      -		rev.reroll_count = reroll_count;
     -+		char *endp;
      +
     -+		reroll_count = strtoul(reroll_count_string, &endp, 10);
     -+		if (!*endp && 0 < reroll_count) {
     -+			previous_count_string = xstrfmt("%d", reroll_count - 1);
     -+		}
     ++		strtol_i(reroll_count_string, 10, &reroll_count);
      +		strbuf_addf(&sprefix, "%s v%s",
      +			    rev.subject_prefix, reroll_count_string);
      +		rev.reroll_count = reroll_count_string;
       		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
       	}
       
     -@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
     - 			die(_("--interdiff requires --cover-letter or single patch"));
     - 		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
     - 		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
     --		rev.idiff_title = diff_title(&idiff_title, reroll_count,
     --					     _("Interdiff:"),
     --					     _("Interdiff against v%d:"));
     -+		rev.idiff_title = diff_title(&idiff_title, reroll_count_string,
     -+					previous_count_string,
     -+					_("Interdiff:"),
     -+					_("Interdiff against v%s:"));
     - 	}
     - 
     - 	if (creation_factor < 0)
     -@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
     - 		rev.rdiff1 = rdiff1.buf;
     - 		rev.rdiff2 = rdiff2.buf;
     - 		rev.creation_factor = creation_factor;
     --		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
     --					     _("Range-diff:"),
     --					     _("Range-diff against v%d:"));
     -+		rev.rdiff_title = diff_title(&rdiff_title, reroll_count_string,
     -+					previous_count_string,
     -+					_("Range-diff:"),
     -+					_("Range-diff against v%s:"));
     - 	}
     - 
     - 	if (!signature) {
      
       ## log-tree.c ##
      @@ log-tree.c: void fmt_output_subject(struct strbuf *filename,


 Documentation/git-format-patch.txt |  1 +
 builtin/log.c                      | 13 ++++++++-----
 log-tree.c                         |  4 ++--
 revision.h                         |  2 +-
 t/t3206-range-diff.sh              | 16 ++++++++++++++++
 t/t4014-format-patch.sh            | 26 ++++++++++++++++++++++++++
 6 files changed, 54 insertions(+), 8 deletions(-)


base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07

Comments

Eric Sunshine March 15, 2021, 11:41 p.m. UTC | #1 
On Fri, Mar 5, 2021 at 2:10 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 sometimes a same 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.
>
> `<n>` can be any string, such as `-v1.1`.  In the case where it
> is a non-integral value, the "Range-diff" and "Interdiff"
> headers will not include the previous version.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -221,6 +221,7 @@ populated with placeholder text.
>         `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
>         file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> +       now can support non-integrated version number like `-v1.1`.

Let's drop "now" from the beginning of the sentence since it is only
meaningful for people who have read this documentation previously, but
not for people newly learning about the option. Perhaps just say:

    `<n>` may be a fractional number.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1862,11 +1863,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> -       if (0 < reroll_count) {
> +       if (reroll_count_string) {
>                 struct strbuf sprefix = STRBUF_INIT;
> +
> +               strtol_i(reroll_count_string, 10, &reroll_count);
> +               strbuf_addf(&sprefix, "%s v%s",
> +                           rev.subject_prefix, reroll_count_string);
> +               rev.reroll_count = reroll_count_string;

This code can be confusing to readers since it appears to ignore the
result of strtol_i(), and it's difficult for the reader to understand
the difference between `reroll_count` and `reroll_count_string` and
why you need both variables. I was going to suggest that you write an
/* in-code comment */ here explaining why you don't care if the reroll
count parsed correctly as a number. However, now that I'm examining
the code again, I think it would be clearer if you move the strtol_i()
call into the diff_title() function since -- following your changes --
that function is the only code which cares whether `reroll_count` can
be parsed as a number (the rest of the code, after your change, just
uses it as a string).
ZheNing Hu March 16, 2021, 5:48 a.m. UTC | #2 
Eric Sunshine <sunshine@sunshineco.com> 于2021年3月16日周二 上午7:41写道：
>
> On Fri, Mar 5, 2021 at 2:10 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 sometimes a same 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.
> >
> > `<n>` can be any string, such as `-v1.1`.  In the case where it
> > is a non-integral value, the "Range-diff" and "Interdiff"
> > headers will not include the previous version.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > @@ -221,6 +221,7 @@ populated with placeholder text.
> >         `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> >         file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> > +       now can support non-integrated version number like `-v1.1`.
>
> Let's drop "now" from the beginning of the sentence since it is only
> meaningful for people who have read this documentation previously, but
> not for people newly learning about the option. Perhaps just say:
>
>     `<n>` may be a fractional number.
>
I agree with you.
> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1862,11 +1863,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > -       if (0 < reroll_count) {
> > +       if (reroll_count_string) {
> >                 struct strbuf sprefix = STRBUF_INIT;
> > +
> > +               strtol_i(reroll_count_string, 10, &reroll_count);
> > +               strbuf_addf(&sprefix, "%s v%s",
> > +                           rev.subject_prefix, reroll_count_string);
> > +               rev.reroll_count = reroll_count_string;
>
> This code can be confusing to readers since it appears to ignore the
> result of strtol_i(), and it's difficult for the reader to understand
> the difference between `reroll_count` and `reroll_count_string` and
> why you need both variables. I was going to suggest that you write an
> /* in-code comment */ here explaining why you don't care if the reroll
> count parsed correctly as a number. However, now that I'm examining
> the code again, I think it would be clearer if you move the strtol_i()
> call into the diff_title() function since -- following your changes --
> that function is the only code which cares whether `reroll_count` can
> be parsed as a number (the rest of the code, after your change, just
> uses it as a string).

Well, The reason `strtol_i` does not check the return value is that
`strtol_i` will
only modify the value of `reroll_count` if the `reroll_count_string`
we provide is
 an integer string, so if `reroll_count_string` is not an integer string, then
`reroll_count` will remain -1, and then `diff_title` will only execute

        if (reroll_count <= 0)
                strbuf_addstr(sb, generic);

So don't need check strtol_i return value. But what you said to put
`strtol_i` in
`diff_title` is indeed a good idea. Of course, we need to modify the declaration
 and parameters of `diff_title`.
Eric Sunshine March 16, 2021, 6:15 a.m. UTC | #3 
On Tue, Mar 16, 2021 at 1:48 AM ZheNing Hu <adlternative@gmail.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> 于2021年3月16日周二 上午7:41写道：
> > > +               strtol_i(reroll_count_string, 10, &reroll_count);
> >
> > This code can be confusing to readers since it appears to ignore the
> > result of strtol_i() [...]
> > [...] I think it would be clearer if you move the strtol_i()
> > call into the diff_title() function since -- following your changes --
> > that function is the only code which cares whether `reroll_count` can
> > be parsed as a number (the rest of the code, after your change, just
> > uses it as a string).
>
> Well, The reason `strtol_i` does not check the return value is that
> `strtol_i` will
> only modify the value of `reroll_count` if the `reroll_count_string`
> we provide is
>  an integer string, so if `reroll_count_string` is not an integer string, then
> `reroll_count` will remain -1, and then `diff_title` will only execute
>
>         if (reroll_count <= 0)
>                 strbuf_addstr(sb, generic);
>
> So don't need check strtol_i return value.

Yes, I understand the reason, but it is not easy for someone new to
this code to figure it out without looking at diff_title() and its
callers. The place where the return value of strtol_i() is ignored is
very far removed from the place where the value of `reroll_count` is
consumed in diff_title(), so it's not obvious to the reader how this
is all supposed to work. If you move strtol_i() into diff_title(),
then the parsing and the consuming of that value happen in the same
place, making it easier to understand.

> But what you said to put
> `strtol_i` in
> `diff_title` is indeed a good idea. Of course, we need to modify the declaration
>  and parameters of `diff_title`.

Yes, it is a very minor modification to diff_title(), changing
`reroll_count` from int to string.

Moving the parsing into diff_title() also allows you to use the
simpler name `reroll_count` for the string variable in cmd_fmt_patch()
rather than the longer `reroll_count_string`; just change
`reroll_count` from int to string.
Junio C Hamano March 17, 2021, 5:27 p.m. UTC | #4 
Eric Sunshine <sunshine@sunshineco.com> writes:

> Yes, it is a very minor modification to diff_title(), changing
> `reroll_count` from int to string.
>
> Moving the parsing into diff_title() also allows you to use the
> simpler name `reroll_count` for the string variable in cmd_fmt_patch()
> rather than the longer `reroll_count_string`; just change
> `reroll_count` from int to string.

It does clarify that the "string" version is the authoritative and
features that depend on being able to learn what the previous
iteration is is available only it is given an integral version
number.

It made a ton of sense when the code allowed an explicit "previous
iteration" to be given by the user also as a string to compute the
previous version with strtol_i() dance, so that diff_title() can be
a mere consumer of both string versions (the current one and the
previous one) without having to worry about how the previous one is
computed.  But without the "previous iteration" string carried
through the codebase as another first-class citizen just like the
"current iteration" string, it does not.

I actually think you would regret not allowing the explicit
"previous iteration" string from the command line and make it
another first-class citizen, but that is a different story.
diff mbox series

Patch

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 3e49bf221087..8af0d2923118 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.
+	now can support non-integrated version number like `-v1.1`.
 
 --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..03be9675fc5e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1718,6 +1718,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int use_patch_format = 0;
 	int quiet = 0;
 	int reroll_count = -1;
+	const char *reroll_count_string = NULL;
 	char *cover_from_description_arg = NULL;
 	char *branch_name = NULL;
 	char *base_commit = NULL;
@@ -1751,7 +1752,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_string, 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,11 +1863,13 @@  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_string) {
 		struct strbuf sprefix = STRBUF_INIT;
-		strbuf_addf(&sprefix, "%s v%d",
-			    rev.subject_prefix, reroll_count);
-		rev.reroll_count = reroll_count;
+
+		strtol_i(reroll_count_string, 10, &reroll_count);
+		strbuf_addf(&sprefix, "%s v%s",
+			    rev.subject_prefix, reroll_count_string);
+		rev.reroll_count = reroll_count_string;
 		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