format-patch: allow a non-integral version numbers

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.

Signed-off-by ZheNing Hu <[email protected]>

---

Documentation/git-format-patch.txt +0 -0

@@ -215,12 +215,12 @@ populated with placeholder text.
 -v <n>::

builtin/log.c +1 -1

@@ -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,

Quoted 4 lines · ZheNing Hu via GitGitGadget

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

Quoted 13 lines

Signed-off-by ZheNing Hu <[email protected]>

---

Documentation/git-format-patch.txt +1 -3

@@ -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.

Documentation/git-format-patch.txt populated with placeholder text. · line 223

Quoted 2 lines

Documentation/git-format-patch.txt +2 -0

+       `--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.

Quoted 5 lines

builtin/log.c +1 -1

@@ -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,

builtin/log.c static void print_bases(struct base_tree_info *bases, FILE *file) · line 1670

Quoted 10 lines

builtin/log.c +2 -2

                       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"

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2099

Quoted 6 lines

builtin/log.c +2 -2

@@ -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".

From: ZheNing Hu <[email protected]>

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.

Signed-off-by ZheNing Hu <[email protected]>

---
[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(-)

Documentation/git-format-patch.txt +3 -3

index 3e49bf221087..0cc39dbf573c 100644
@@ -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

builtin/log.c +10 -10

index f67b67d80ed1..72af140b812e 100644
@@ -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) {

log-tree.c +2 -2

index 4531cebfab38..5f2e08ebcaab 100644
@@ -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)

revision.h +1 -1

index e6be3c845e66..097d08354c61 100644
@@ -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;

t/t4014-format-patch.sh +4 -4

index 66630c8413d5..b911e08f0810 100755
@@ -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

Quoted 2 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Quoted 4 lines

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.

Quoted 169 lines

Signed-off-by ZheNing Hu <[email protected]>

---
[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(-)

Documentation/git-format-patch.txt +3 -3

index 3e49bf221087..0cc39dbf573c 100644
@@ -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

builtin/log.c +10 -10

index f67b67d80ed1..72af140b812e 100644
@@ -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) {

log-tree.c +2 -2

index 4531cebfab38..5f2e08ebcaab 100644
@@ -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)

revision.h +1 -1

index e6be3c845e66..097d08354c61 100644
@@ -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;

t/t4014-format-patch.sh +4 -4

index 66630c8413d5..b911e08f0810 100755
@@ -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

Quoted 2 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Quoted 4 lines

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.

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.

Quoted 6 lines

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.

builtin/log.c +3 -0

+	if (previous_count && !reroll_count)
+		usage(_("previous-count can only used when reroll-count is used"));
+	if (reroll_count) {
 		struct strbuf sprefix = STRBUF_INIT;

From: ZheNing Hu <[email protected]>

Usually we can only use format-patch -v<n> to generate integral version numbers patches, but sometimes a same fixup should be laveled as a non-integral versions like v1.1,so teach format-patch

allow a non-integral versions 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 provide format patch a new option --previous-count=<n>, the patch can output user-specified previous version number. If the user use a integral version number -v<n>, ensure that the output in the patch is still v<n-1>. (let --previous-count` become invalid.)

Signed-off-by ZheNing Hu <[email protected]>

---

Documentation/git-format-patch.txt +10 -0

index 3e49bf221087..f10fa6527f5f 100644
@@ -22,6 +22,7 @@ SYNOPSIS
 		   [--cover-from-description=<mode>]
 		   [--rfc] [--subject-prefix=<subject prefix>]
 		   [(--reroll-count|-v) <n>]
+		   [--previous-count=<n>]
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet]
 		   [--[no-]encode-email-headers]

@@ -221,6 +222,15 @@ 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`.
+
+--previous-count=<n>::
+	Under the premise that we have used `--reroll-count=<n>`,
+	we can use `--previous-count=<n>` to specify the previous
+	version number. E.g. When we use the `--range-diff` or
+	`--interdiff` option and combine with `-v2.3 --previous-count=2.2`,
+	"Interdiff against v2.2:" or "Range-diff against v2.2:"
+	will be output in the patch.
 
 --to=<email>::
 	Add a `To:` header to the email headers. This is in addition

builtin/log.c +2 -3

index f67b67d80ed1..95c95eab5393 100644
@@ -1662,13 +1662,15 @@ 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, int reroll_count_is_integer,
+			const char*previous_count, const char *generic, const char *rerolled)
 {
-	if (reroll_count <= 0)

builtin/log.c +10 -5

 	char *cover_from_description_arg = NULL;
 	char *branch_name = NULL;
 	char *base_commit = NULL;

@@ -1751,8 +1755,10 @@ 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_STRING(0, "previous-count", &previous_count, N_("previous-count"),
+			    N_("specified as the last version while we use --reroll-count")),
 		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
 			    N_("max length of output filename")),
 		OPT_CALLBACK_F(0, "rfc", &rev, NULL,

@@ -1861,10 +1867,20 @@ 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 (previous_count && !reroll_count)
+		usage(_("previous-count can only used when reroll-count is used"));
+	if (reroll_count) {
 		struct strbuf sprefix = STRBUF_INIT;
-		strbuf_addf(&sprefix, "%s v%d",
+		char ch;
+		size_t i = 0 , reroll_count_len = strlen(reroll_count);
+

Hmm, others may disagree but I don't really like the idea of --previous-count. It may be useful for populating "Range-diff vs <n>" instead of just "Range-diff" but I don't think it's worth the cost of maintaining this option.

Quoted 4 lines · Denton Liu

Hmm, others may disagree but I don't really like the idea of --previous-count. It may be useful for populating "Range-diff vs <n>" instead of just "Range-diff" but I don't think it's worth the cost of maintaining this option.

It really depends on the target audience. As a reviewer who may be too busy to read every iteration of a series, I would probably find it useless if it gives just "range-diff" or "range-diff with last" without saying which exact round. Obviously, if you are not doing range-diff, it will not be an issue. If the patch requires (I didn't read the latest one) the previous-count to be given when range-diff or interdiff is not requested, it should probably be fixed.

I am also OK with any design decision, as long as it will not close the door for the occasionally requested feature to carry over cover letter material from the previous round to the current one.

Thanks.

What we are arguing now is whether it is necessary to add "aginst v<previous_count>" to the patch when the non-integer version number + rangediff/interdiff is required. Denton's point of view may be similar to that of Eric before.

Here are my personal thoughts:

Of course this previous count can be used in a very small range, but I think it
doesn't hurt to keep it, because even if you don't use it, `format
patch` will still
output "Range-diff", which will not break any known functions. It can only be said
that previous count provides an option for submitters to know the previous version
for reviewers. In this regard, I agree with Junio's point of view.

I'm not outright opposed to supporting non-numeric, non-integer reroll-counts, but I also don't see a big need for it. As mentioned earlier, Denton is the only person I recall who sends fractional re-rolls, so it's not obvious that there is a big advantage to adding such support and complicating the code just for one person.

From: ZheNing Hu <[email protected]>

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.

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".

Signed-off-by ZheNing Hu <[email protected]>

---
[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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/885

Range-diff vs v2:

1: 800094cbf53b ! 1: d4f38b78c464 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
     -    laveled as a non-integral versions like `v1.1`,so teach format-patch
     -    allow a non-integral versions may be helpful to send those patches.
     +    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.
      
          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 provide
     -    `format patch` a new option `--previous-count=<n>`, the patch can output
     -    user-specified previous version number. If the user use a integral version
     -    number `-v<n>`, ensure that the output in the patch is still `v<n-1>`.
     -    (let `--previous-count` become invalid.)
     +    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".
      
          Signed-off-by: ZheNing Hu <[email protected]>
      
       ## Documentation/git-format-patch.txt ##
     -@@ Documentation/git-format-patch.txt: SYNOPSIS
     - 		   [--cover-from-description=<mode>]
     - 		   [--rfc] [--subject-prefix=<subject prefix>]
     - 		   [(--reroll-count|-v) <n>]
     -+		   [--previous-count=<n>]
     - 		   [--to=<email>] [--cc=<email>]
     - 		   [--[no-]cover-letter] [--quiet]
     - 		   [--[no-]encode-email-headers]
      @@ Documentation/git-format-patch.txt: 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`.
     -+
     -+--previous-count=<n>::
     -+	Under the premise that we have used `--reroll-count=<n>`,
     -+	we can use `--previous-count=<n>` to specify the previous
     -+	version number. E.g. When we use the `--range-diff` or
     -+	`--interdiff` option and combine with `-v2.3 --previous-count=2.2`,
     -+	"Interdiff against v2.2:" or "Range-diff against v2.2:"
     -+	will be output in the patch.
       
       --to=<email>::
       	Add a `To:` header to the email headers. This is in addition
     @@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file)
       
      -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, int reroll_count_is_integer,
     -+			const char*previous_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 || (!reroll_count_is_integer && !previous_count))
     ++	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 (reroll_count_is_integer)/* RFC may be v0, so allow -v1 to diff against v0 */
     -+		strbuf_addf(sb, rerolled, atoi(reroll_count) - 1);
     -+	else if (previous_count)
     -+		strbuf_addf(sb, rerolled, previous_count);
     ++	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)
     - 	struct strbuf buf = STRBUF_INIT;
       	int use_patch_format = 0;
       	int quiet = 0;
     --	int reroll_count = -1;
     -+	int reroll_count_is_integer = 0;
     -+	const char *reroll_count = NULL;
     -+	const char *previous_count = NULL;
     + 	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
       			    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"),
     ++		OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
      +			    N_("mark the series as specified version re-roll")),
     -+		OPT_STRING(0, "previous-count", &previous_count, N_("previous-count"),
     -+			    N_("specified as the last version while we use --reroll-count")),
       		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);
     --
     + 
      -	if (0 < reroll_count) {
     -+	if (previous_count && !reroll_count)
     -+		usage(_("previous-count can only used when reroll-count is used"));
     -+	if (reroll_count) {
     ++	if (reroll_count_string) {
       		struct strbuf sprefix = STRBUF_INIT;
      -		strbuf_addf(&sprefix, "%s v%d",
     -+		char ch;
     -+		size_t i = 0 , reroll_count_len = strlen(reroll_count);
     +-			    rev.subject_prefix, reroll_count);
     +-		rev.reroll_count = reroll_count;
     ++		char *endp;
      +
     -+		for (; i != reroll_count_len; i++) {
     -+			ch = reroll_count[i];
     -+			if(!isdigit(ch))
     -+				break;
     ++		reroll_count = strtoul(reroll_count_string, &endp, 10);
     ++		if (!*endp && 0 < reroll_count) {
     ++			previous_count_string = xstrfmt("%d", reroll_count - 1);
      +		}
     -+		reroll_count_is_integer = i == reroll_count_len ? 1 : 0;
      +		strbuf_addf(&sprefix, "%s v%s",
     - 			    rev.subject_prefix, reroll_count);
     - 		rev.reroll_count = reroll_count;
     ++			    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,
     +-		rev.idiff_title = diff_title(&idiff_title, reroll_count,
      -					     _("Interdiff:"),
      -					     _("Interdiff against v%d:"));
     -+			reroll_count_is_integer, previous_count, _("Interdiff:"),
     -+				reroll_count_is_integer ? _("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,
     +-		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
      -					     _("Range-diff:"),
      -					     _("Range-diff against v%d:"));
     -+			reroll_count_is_integer, previous_count, _("Range-diff:"),
     -+				reroll_count_is_integer ? _("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:"));
       	}
       
     @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff as comment
       	grep "> 1: .* new message" 0001-*
       '
       
     -+test_expect_success 'format-patch --range-diff reroll-count with a non-integer and previous-count ' '
     -+	git format-patch --range-diff=HEAD~1 -v2.9 --previous-count=2.8 HEAD~1 >actual &&
     ++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.8:$" v2.9-0001-* &&
     ++	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 previous-count' '
     -+	git format-patch --range-diff=HEAD~1 -v2 --previous-count=1.8 HEAD~1 >actual &&
     ++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-* &&
     @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: reroll-count' '
      +	test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
      +'
      +
     -+test_expect_success 'interdiff: reroll-count with a non-integer and previous-count ' '
     -+	git format-patch --cover-letter --interdiff=boop~2 -v2.2 --previous-count=2.1 -1 boop &&
     -+	test_i18ngrep "^Interdiff ..* v2.1:$" v2.2-0000-cover-letter.patch
     -+'
     -+
     -+test_expect_success 'interdiff: reroll-count with a integer and previous-count ' '
     -+	git format-patch --cover-letter --interdiff=boop~2 -v2 --previous-count=1.5 -1 boop &&
     ++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: previous-count without reroll-count ' '
     -+	test_must_fail git format-patch --cover-letter --interdiff=boop~2 --previous-count=1.5 -1 boop
     -+'
     ++
       test_expect_success 'interdiff: solo-patch' '
       	cat >expect <<-\EOF &&
       	  +fleep

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

Documentation/git-format-patch.txt +0 -0

index 3e49bf221087..8af0d2923118 100644

Documentation/git-format-patch.txt +0 -0

 --to=<email>::
 	Add a `To:` header to the email headers. This is in addition

builtin/log.c +4 -2

index f67b67d80ed1..d135c30620b6 100644
@@ -1662,13 +1662,15 @@ 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)

builtin/log.c +2 -0

@@ -1718,6 +1720,8 @@ 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 +0 -0

 		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
 			    N_("max length of output filename")),
 		OPT_CALLBACK_F(0, "rfc", &rev, NULL,

@@ -1862,11 +1866,17 @@ 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 +13 -6

+			previous_count_string = xstrfmt("%d", reroll_count - 1);
+		}
+		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);
 	}
 

@@ -2078,9 +2088,10 @@ 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)

@@ -2097,9 +2108,10 @@ 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 +2 -2

index 4531cebfab38..5f2e08ebcaab 100644
@@ -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)

revision.h +1 -1

index e6be3c845e66..097d08354c61 100644
@@ -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;

t/t3206-range-diff.sh +16 -0

index 1b26c4c2ef91..dc419c087e07 100755
@@ -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^...
 '

t/t4014-format-patch.sh +26 -0

index 66630c8413d5..59dff38065ab 100755
@@ -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

base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07

Signature · 1 line

gitgitgadget

Hi ZheNing,

Quoted 5 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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

s/versions/version/

Quoted 1 line

to allow a non-integral versions may be helpful to send those patches.

s/may be/which &/

Quoted 256 lines

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".

Signed-off-by ZheNing Hu <[email protected]>

---
[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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/885

Range-diff vs v2:

1: 800094cbf53b ! 1: d4f38b78c464 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
- laveled as a non-integral versions like v1.1,so teach format-patch
- allow a non-integral versions may be helpful to send those patches.
+ 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.

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 provide
- format patch a new option --previous-count=<n>, the patch can output
- user-specified previous version number. If the user use a integral version
- number -v<n>, ensure that the output in the patch is still v<n-1>.
- (let --previous-count become invalid.)
+ 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".

Signed-off-by ZheNing Hu <[email protected]>

## Documentation/git-format-patch.txt ##
-@@ Documentation/git-format-patch.txt: SYNOPSIS
- [--cover-from-description=<mode>]
- [--rfc] [--subject-prefix=<subject prefix>]
- [(--reroll-count|-v) <n>]
-+ [--previous-count=<n>]
- [--to=<email>] [--cc=<email>]
- [--[no-]cover-letter] [--quiet]
- [--[no-]encode-email-headers]
@@ Documentation/git-format-patch.txt: 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.
-+
-+--previous-count=<n>::
-+ Under the premise that we have used --reroll-count=<n>,
-+ we can use --previous-count=<n> to specify the previous
-+ version number. E.g. When we use the --range-diff or
-+ --interdiff option and combine with -v2.3 --previous-count=2.2,
-+ "Interdiff against v2.2:" or "Range-diff against v2.2:"
-+ will be output in the patch.

--to=<email>::
Add a To: header to the email headers. This is in addition
@@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file)

-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, int reroll_count_is_integer,
-+ const char*previous_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 || (!reroll_count_is_integer && !previous_count))
++ 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 (reroll_count_is_integer)/* RFC may be v0, so allow -v1 to diff against v0 */
-+ strbuf_addf(sb, rerolled, atoi(reroll_count) - 1);
-+ else if (previous_count)
-+ strbuf_addf(sb, rerolled, previous_count);
++ 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)
- struct strbuf buf = STRBUF_INIT;
int use_patch_format = 0;
int quiet = 0;
-- int reroll_count = -1;
-+ int reroll_count_is_integer = 0;
-+ const char *reroll_count = NULL;
-+ const char *previous_count = NULL;
+ 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
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"),
++ OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
+ N_("mark the series as specified version re-roll")),
-+ OPT_STRING(0, "previous-count", &previous_count, N_("previous-count"),
-+ N_("specified as the last version while we use --reroll-count")),
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);
--
+
- if (0 < reroll_count) {
-+ if (previous_count && !reroll_count)
-+ usage(_("previous-count can only used when reroll-count is used"));
-+ if (reroll_count) {
++ if (reroll_count_string) {
struct strbuf sprefix = STRBUF_INIT;
- strbuf_addf(&sprefix, "%s v%d",
-+ char ch;
-+ size_t i = 0 , reroll_count_len = strlen(reroll_count);
+- rev.subject_prefix, reroll_count);
+- rev.reroll_count = reroll_count;
++ char *endp;
+
-+ for (; i != reroll_count_len; i++) {
-+ ch = reroll_count[i];
-+ if(!isdigit(ch))
-+ break;
++ reroll_count = strtoul(reroll_count_string, &endp, 10);
++ if (!*endp && 0 < reroll_count) {
++ previous_count_string = xstrfmt("%d", reroll_count - 1);
+ }
-+ reroll_count_is_integer = i == reroll_count_len ? 1 : 0;
+ strbuf_addf(&sprefix, "%s v%s",
- rev.subject_prefix, reroll_count);
- rev.reroll_count = reroll_count;
++ 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,
+- rev.idiff_title = diff_title(&idiff_title, reroll_count,
- _("Interdiff:"),
- _("Interdiff against v%d:"));
-+ reroll_count_is_integer, previous_count, _("Interdiff:"),
-+ reroll_count_is_integer ? _("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,
+- rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
- _("Range-diff:"),
- _("Range-diff against v%d:"));
-+ reroll_count_is_integer, previous_count, _("Range-diff:"),
-+ reroll_count_is_integer ? _("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:"));
}

@@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff as comment
grep "> 1: .* new message" 0001-*
'

-+test_expect_success 'format-patch --range-diff reroll-count with a non-integer and previous-count ' '
-+ git format-patch --range-diff=HEAD~1 -v2.9 --previous-count=2.8 HEAD~1 >actual &&
++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.8:$" v2.9-0001-* &&
++ 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 previous-count' '
-+ git format-patch --range-diff=HEAD~1 -v2 --previous-count=1.8 HEAD~1 >actual &&
++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-* &&
@@ t/t4014-format-patch.sh: test_expect_success 'interdiff: reroll-count' '
+ test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
+'
+
-+test_expect_success 'interdiff: reroll-count with a non-integer and previous-count ' '
-+ git format-patch --cover-letter --interdiff=boop~2 -v2.2 --previous-count=2.1 -1 boop &&
-+ test_i18ngrep "^Interdiff ..* v2.1:$" v2.2-0000-cover-letter.patch
-+'
-+
-+test_expect_success 'interdiff: reroll-count with a integer and previous-count ' '
-+ git format-patch --cover-letter --interdiff=boop~2 -v2 --previous-count=1.5 -1 boop &&
++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: previous-count without reroll-count ' '
-+ test_must_fail git format-patch --cover-letter --interdiff=boop~2 --previous-count=1.5 -1 boop
-+'
++
test_expect_success 'interdiff: solo-patch' '
cat >expect <<-\EOF &&
+fleep

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

Documentation/git-format-patch.txt +0 -0

index 3e49bf221087..8af0d2923118 100644

Documentation/git-format-patch.txt populated with placeholder text. · line 224

Quoted 4 lines

Documentation/git-format-patch.txt +1 -0

@@ -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`.

Perhaps something like:

+
<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.

Quoted 21 lines

Documentation/git-format-patch.txt +0 -0

 --to=<email>::
      Add a `To:` header to the email headers. This is in addition

builtin/log.c +5 -4

index f67b67d80ed1..d135c30620b6 100644
@@ -1662,13 +1662,15 @@ 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 */

builtin/log.c static void print_bases(struct base_tree_info *bases, FILE *file) · line 1675

Quoted 5 lines

builtin/log.c +2 -1

-             strbuf_addf(sb, rerolled, reroll_count - 1);
+     else if (previous_count_string)
+             strbuf_addf(sb, rerolled, previous_count_string);
      return sb->buf;
 }

I don't think it's necessary to do this at all. We can just leave reroll_count < 0 here.

Quoted 13 lines

builtin/log.c +2 -0

@@ -1718,6 +1720,8 @@ 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;

@@ -1751,8 +1755,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,

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1759

Quoted 5 lines

builtin/log.c +2 -2

                          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")),

Others may disagree but I'm okay with leaving this as "Nth re-roll". It's just a synopsis. More information can be found in the docs.

Quoted 8 lines

builtin/log.c +0 -0

              OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
                          N_("max length of output filename")),
              OPT_CALLBACK_F(0, "rfc", &rev, NULL,

@@ -1862,11 +1866,17 @@ 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 *prefix) · line 1874

Quoted 10 lines

builtin/log.c +5 -4

-     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;
+             char *endp;
+
+             reroll_count = strtoul(reroll_count_string, &endp, 10);
+             if (!*endp && 0 < reroll_count) {

This 0 < reroll_count check is unnecessary; it was initialised to -1 and it hasn't changed since here.

Also, we can take advantage of the strtol_i() function, which can perform bounds checking and error checking for us. This allows us to assign reroll_count only when a valid integer is found so we can eliminate previous_count_string().

Something like:

        strtol_i(reroll_count_string, 10, &reroll_count);

Quoted 146 lines

builtin/log.c +13 -6

+                     previous_count_string = xstrfmt("%d", reroll_count - 1);
+             }
+             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);
      }
 

@@ -2078,9 +2088,10 @@ 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)

@@ -2097,9 +2108,10 @@ 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 +2 -2

index 4531cebfab38..5f2e08ebcaab 100644
@@ -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)

revision.h +1 -1

index e6be3c845e66..097d08354c61 100644
@@ -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;

t/t3206-range-diff.sh +16 -0

index 1b26c4c2ef91..dc419c087e07 100755
@@ -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^...
 '

t/t4014-format-patch.sh +26 -0

index 66630c8413d5..59dff38065ab 100755
@@ -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

base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07 --
gitgitgadget

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 <[email protected]>

---

Quoted 14 lines · ZheNing Hu via GitGitGadget

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 <[email protected]>

---

Documentation/git-format-patch.txt populated with placeholder text. · line 223

Quoted 3 lines

Documentation/git-format-patch.txt +1 -0

@@ -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.

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1869

Quoted 10 lines

builtin/log.c +6 -1

@@ -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).

Quoted 9 lines · Eric Sunshine <[email protected]> 于2021年3月16日周二 上午7:41写道：

Quoted 1 line

+ 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.

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.

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 <[email protected]>

---

builtin/log.c +4 -0

@@ -1662,13 +1662,19 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
+static const char *diff_title(struct strbuf *sb,
+                             const char *reroll_count,
+                             const char *generic,
+                             const char *rerolled)
 {

Quoted 19 lines · ZheNing Hu via GitGitGadget

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 <[email protected]>

---

builtin/log.c +4 -0

@@ -1662,13 +1662,19 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
+static const char *diff_title(struct strbuf *sb,
+                             const char *reroll_count,
+                             const char *generic,
+                             const char *rerolled)
 {

builtin/log.c static void print_bases(struct base_tree_info *bases, FILE *file) · line 1676

Quoted 10 lines

builtin/log.c +6 -0

+       int reroll_count_int = -1;
+
+       if (reroll_count)
+               strtol_i(reroll_count, 10, &reroll_count_int);
+       if (reroll_count_int <= 0)
                strbuf_addstr(sb, generic);
        else /* RFC may be v0, so allow -v1 to diff against v0 */
+               strbuf_addf(sb, rerolled, reroll_count_int - 1);
        return sb->buf;
 }

Thanks. The logic of this version is much easier to understand now that the number parsing has been moved into diff_title().

It may still be a bit confusing for someone reading this code to understand why you don't check the return value of strtol_i(). Therefore, it might be a good idea to add an /* in-code comment */ explaining why you don't check whether the parse succeeded or failed. However, if we rewrite the code like this:

    int v;
    if (reroll_count && !strtol_i(reroll_count, 10, &v))
        strbuf_addf(sb, rerolled, v - 1);
    else
        strbuf_addstr(sb, generic);
    return sb->buf;

then the logic becomes obvious, and we don't even need a comment. (Notice that I also shortened the variable name since the code is just as clear with a short name as with a long spelled out name such as "reroll_count_int".)

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 <[email protected]>

---

builtin/log.c +5 -1

@@ -1662,13 +1662,18 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
+static const char *diff_title(struct strbuf *sb,
+                             const char *reroll_count,
+                             const char *generic,
+                             const char *rerolled)
 {
-       if (reroll_count <= 0)
+       int v;

Quoted 21 lines · ZheNing Hu via GitGitGadget

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 <[email protected]>

---

builtin/log.c +5 -1

@@ -1662,13 +1662,18 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
+static const char *diff_title(struct strbuf *sb,
+                             const char *reroll_count,
+                             const char *generic,
+                             const char *rerolled)
 {
-       if (reroll_count <= 0)
+       int v;

builtin/log.c static void print_bases(struct base_tree_info *bases, FILE *file) · line 1675

Quoted 10 lines

builtin/log.c +5 -2

+
+       /* RFC may be v0, so allow -v1 to diff against v0 */
+       if (reroll_count && !strtol_i(reroll_count, 10, &v))
+               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;
 }

The comment about RFC and v0 doesn't really make sense anymore. Its original purpose was to explain why the if condition (which goes away with this patch) was <=0 rather than <=1. It might make sense to keep the comment if the code is written like this:

    if (reroll_count &&
            !strtol_i(reroll_count, 10, &v) &&
            reroll_count >= 1)
        strbuf_addf(sb, rerolled, v - 1);
    else
        ...

However, I'm not sure it's worth re-rolling just to make this change.

Thanks.

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?

Quoted 4 lines · ZheNing Hu via GitGitGadget

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".

From: ZheNing Hu <[email protected]>

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.

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);

Quoted 4 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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?

Quoted 4 lines

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.

Quoted 1 line

<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.

Quoted 3 lines

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).

Quoted 13 lines

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.

From: ZheNing Hu <[email protected]>

Usually we can only use format-patch -v<n> to generate integral version numbers patches,

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.

<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.

Signed-off-by ZheNing Hu <[email protected]>

---
[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?

"ZheNing Hu via GitGitGadget" <[email protected]> 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 ;-)

Quoted 5 lines

From: ZheNing Hu <[email protected]>

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.

Quoted 4 lines

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.

Quoted 3 lines

<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.

Quoted 4 lines

Signed-off-by ZheNing Hu <[email protected]>

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

Quoted 4 lines

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?

Documentation/git-format-patch.txt populated with placeholder text. · line 224

Quoted 9 lines

Documentation/git-format-patch.txt +1 -0

index 3e49bf221087..e2c29a856451 100644
@@ -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?

Documentation/git-format-patch.txt populated with placeholder text. · line 228

Quoted 4 lines

Documentation/git-format-patch.txt +4 -0

+     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.

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".

log-tree.c void fmt_output_subject(struct strbuf *filename, · line 371

Quoted 5 lines

log-tree.c +2 -0

@@ -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.)

Quoted 4 lines · Eric Sunshine

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.

Quoted 12 lines · Junio C Hamano

Quoted 4 lines · Eric Sunshine

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.

Quoted 2 lines

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

Documentation/git-format-patch.txt populated with placeholder text. · line 225

Quoted 7 lines · ZheNing Hu via GitGitGadget

Documentation/git-format-patch.txt +5 -0

@@ -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".

log-tree.c void fmt_output_subject(struct strbuf *filename, · line 371

Quoted 5 lines

log-tree.c +2 -0

@@ -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.)