Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[Jeff King]

On Thu, Nov 05, 2020 at 08:15:48PM +0000, Hu Keping wrote:

> The file name of the patch generated by `git format-patch` usually be
> truncated as there is less than 60 bytes left for the subject of commit
> message exclude the prefix patch number, say "0001-".
> 
> As per the kernel documentation[1], it suggest the length of subject no
> more than 75.
> >
> > [...]
> > For these reasons, the ``summary`` must be no more than 70-75
> > characters, and it must describe both what the patch changes, as well
> > as why the patch might be necessary.
> >
> 
> Considered the prefix patch number "0001-" would take 5 characters, increase
> the FORMAT_PATCH_NAME_MAX to 80.

As the code is written now, the length also includes the ".patch"
suffix, as well as an extra byte (maybe for a NUL? Once upon a time I
imagine we used static buffers, but these days it's all in a strbuf).

A simple test with:

  git init
  for i in $(seq 8); do printf 1234567890; done |
  git commit --allow-empty -F -
  git format-patch -1

shows us generating:

  0001-1234567890123456789012345678901234567890123456789012.patch

So that's only 52 characters, from our constant of 64. Bumping to 80
gives us 66, which is reasonable though probably still involves
occasional truncation. But maybe keeping the total length to 80 (79,
really, because of the extra byte) may be worth doing.

Which is all a long-winded way of saying that your patch seems
reasonable to me.

Looking at the code which uses the constant, I suspect it could also be
made simpler:

  - the PATH_MAX check in open_next_file() seems pointless. Once upon a
    time it mattered for fitting into a PATH_MAX buffer, but these days
    we use a dynamic buffer anyway. We are probably better off to just
    feed the result to the filesystem and see if it complains (since
    either way we are aborting; I'd feel differently if we adjusted our
    truncation size)

  - the logic in fmt_output_subject() could probably be simpler if the
    constant was "here's how long the subject should be", not "here's
    how long the whole thing must be".

But those are both orthogonal to your patch and can be done separately.

-Peff

[PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[Hu Keping]

The file name of the patch generated by `git format-patch` usually be
truncated as there is less than 60 bytes left for the subject of commit
message exclude the prefix patch number, say "0001-".

As per the kernel documentation[1], it suggest the length of subject no
more than 75.
>
> [...]
> For these reasons, the ``summary`` must be no more than 70-75
> characters, and it must describe both what the patch changes, as well
> as why the patch might be necessary.
>

Considered the prefix patch number "0001-" would take 5 characters, increase
the FORMAT_PATCH_NAME_MAX to 80.

[1] https://www.kernel.org/doc/Documentation/process/submitting-patches.rst

Signed-off-by: Hu Keping <hukeping@huawei.com>
---
 log-tree.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/log-tree.h b/log-tree.h
index 8fa79289ec..021aee0d21 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -33,7 +33,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
 
-#define FORMAT_PATCH_NAME_MAX 64
+#define FORMAT_PATCH_NAME_MAX 80
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
-- 
2.18.0.huawei.25

Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Considered the prefix patch number "0001-" would take 5 characters, increase
>> the FORMAT_PATCH_NAME_MAX to 80.
>
> As the code is written now, the length also includes the ".patch"
> suffix, as well as an extra byte (maybe for a NUL? Once upon a time I
> imagine we used static buffers, but these days it's all in a strbuf).
>
> A simple test with:
>
>   git init
>   for i in $(seq 8); do printf 1234567890; done |
>   git commit --allow-empty -F -
>   git format-patch -1
>
> shows us generating:
>
>   0001-1234567890123456789012345678901234567890123456789012.patch
>
> So that's only 52 characters, from our constant of 64. Bumping to 80
> gives us 66, which is reasonable though probably still involves
> occasional truncation. But maybe keeping the total length to 80 (79,
> really, because of the extra byte) may be worth doing.
>
> Which is all a long-winded way of saying that your patch seems
> reasonable to me.

A devil's advocate thinks that we should shorten it (and rename it
to format-patch-subject-prefix-length or something) instead.  That
way, "ls" output can show more than one files on a single line even
on a 80-column terminal.  The leading digits already guarantee the
uniqueness anyway.

I do not mind getting rid of the "FORMAT_PATCH_NAME_MAX" constant
and replacing it with a variable that defaults to 64 and can be
tweaked by a command line option and/or a configuration variable.
It does not feel it is worth the effort to replace one hardcoded
constant with another hardcoded constant.

> Looking at the code which uses the constant, I suspect it could also be
> made simpler:
>
>   - the PATH_MAX check in open_next_file() seems pointless. Once upon a
>     time it mattered for fitting into a PATH_MAX buffer, but these days
>     we use a dynamic buffer anyway. We are probably better off to just
>     feed the result to the filesystem and see if it complains (since
>     either way we are aborting; I'd feel differently if we adjusted our
>     truncation size)
>
>   - the logic in fmt_output_subject() could probably be simpler if the
>     constant was "here's how long the subject should be", not "here's
>     how long the whole thing must be".
>
> But those are both orthogonal to your patch and can be done separately.

Yes, these clean-ups seem worth doing.

RE: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[hukeping]

>-----Original Message-----
>From: Junio C Hamano [mailto:gitster@pobox.com]
>Sent: Friday, November 6, 2020 5:17 AM
>To: Jeff King <peff@peff.net>
>Cc: hukeping <hukeping@huawei.com>; git@vger.kernel.org; Zhengjunling (JRing,
>Task Force) <zhengjunling@huawei.com>; zhuangbiaowei
><zhuangbiaowei@huawei.com>; git@stormcloud9.net; rafa.almas@gmail.com;
>l.s.r@web.de
>Subject: Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80
>
>Jeff King <peff@peff.net> writes:
>
>>> Considered the prefix patch number "0001-" would take 5 characters,
>>> increase the FORMAT_PATCH_NAME_MAX to 80.
>>
>> As the code is written now, the length also includes the ".patch"
>> suffix, as well as an extra byte (maybe for a NUL? Once upon a time I
>> imagine we used static buffers, but these days it's all in a strbuf).
>>
>> A simple test with:
>>
>>   git init
>>   for i in $(seq 8); do printf 1234567890; done |
>>   git commit --allow-empty -F -
>>   git format-patch -1
>>
>> shows us generating:
>>
>>   0001-1234567890123456789012345678901234567890123456789012.patch
>>
>> So that's only 52 characters, from our constant of 64. Bumping to 80
>> gives us 66, which is reasonable though probably still involves
>> occasional truncation. But maybe keeping the total length to 80 (79,
>> really, because of the extra byte) may be worth doing.
>>
>> Which is all a long-winded way of saying that your patch seems
>> reasonable to me.
>
>A devil's advocate thinks that we should shorten it (and rename it to format-
>patch-subject-prefix-length or something) instead.  That way, "ls" output can
>show more than one files on a single line even on a 80-column terminal.  The
>leading digits already guarantee the uniqueness anyway.
>
>I do not mind getting rid of the "FORMAT_PATCH_NAME_MAX" constant and
>replacing it with a variable that defaults to 64 and can be tweaked by a command
>line option and/or a configuration variable.
>It does not feel it is worth the effort to replace one hardcoded constant with
>another hardcoded constant.
>
>> Looking at the code which uses the constant, I suspect it could also
>> be made simpler:
>>
>>   - the PATH_MAX check in open_next_file() seems pointless. Once upon a
>>     time it mattered for fitting into a PATH_MAX buffer, but these days
>>     we use a dynamic buffer anyway. We are probably better off to just
>>     feed the result to the filesystem and see if it complains (since
>>     either way we are aborting; I'd feel differently if we adjusted our
>>     truncation size)
>>
>>   - the logic in fmt_output_subject() could probably be simpler if the
>>     constant was "here's how long the subject should be", not "here's
>>     how long the whole thing must be".
>>
>> But those are both orthogonal to your patch and can be done separately.
>
>Yes, these clean-ups seem worth doing.

Agreed, and I'd like to do it with two separated commits:
- commit-1,  cleanup the open_next_file() by drop the if (filename.len>=..) statements.

- commit-2,  replace FORMAT_PATCH_NAME_MAX in fmt_output_subject() with a constant
  in there and make it to 80(or other value?), and drop FORMAT_PATCH_NAME_MAX
  from log-tree.h.

Is this works for you?

Thanks.

Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[Junio C Hamano]

hukeping <hukeping@huawei.com> writes:

>>I do not mind getting rid of the "FORMAT_PATCH_NAME_MAX" constant and
>>replacing it with a variable that defaults to 64 and can be tweaked by a command
>>line option and/or a configuration variable.
>>It does not feel it is worth the effort to replace one hardcoded constant with
>>another hardcoded constant.
>>
>>> Looking at the code which uses the constant, I suspect it could also
>>> be made simpler:
>>>
>>>   - the PATH_MAX check in open_next_file() seems pointless. Once upon a
>>>     time it mattered for fitting into a PATH_MAX buffer, but these days
>>>     we use a dynamic buffer anyway. We are probably better off to just
>>>     feed the result to the filesystem and see if it complains (since
>>>     either way we are aborting; I'd feel differently if we adjusted our
>>>     truncation size)
>>>
>>>   - the logic in fmt_output_subject() could probably be simpler if the
>>>     constant was "here's how long the subject should be", not "here's
>>>     how long the whole thing must be".
>>>
>>> But those are both orthogonal to your patch and can be done separately.
>>
>>Yes, these clean-ups seem worth doing.
>
> Agreed, and I'd like to do it with two separated commits:
> - commit-1,  cleanup the open_next_file() by drop the if (filename.len>=..) statements.
>
> - commit-2,  replace FORMAT_PATCH_NAME_MAX in fmt_output_subject() with a constant
>   in there and make it to 80(or other value?), and drop FORMAT_PATCH_NAME_MAX
>   from log-tree.h.
>
> Is this works for you?

I am not sure what you meant by "Agreed".  I said two things:

 - It is dubious that it is worth the effort to replace a hardcoded
   constant with another.  Making it configurable with command line
   option and/or configuration variable may be worth doing.

 - Two observations Peff made for further clean-up are probably
   worth doing.

If you are agreeing to both of the above and following through, then
yes, it seems like a good plan.  If agreement is only to the former,
it probably still is worth doing.  Anything else, I don't know.

Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> A devil's advocate thinks that we should shorten it (and rename it
> to format-patch-subject-prefix-length or something) instead.  That
> way, "ls" output can show more than one files on a single line even
> on a 80-column terminal.  The leading digits already guarantee the
> uniqueness anyway.
>
> I do not mind getting rid of the "FORMAT_PATCH_NAME_MAX" constant
> and replacing it with a variable that defaults to 64 and can be
> tweaked by a command line option and/or a configuration variable.
> It does not feel it is worth the effort to replace one hardcoded
> constant with another hardcoded constant.

So here is my lunch-time hack for the day.  Totally untested beyond
"it comiples and links".

A new configuration variable format.patchnamemax and a new command
line option --filename-max-length=<n> overrides the hardcoded
constant 64, which is the default.  A value that is unreasonably
small is corrected to a hardcoded floor value of 8.

No new tests added nor documentation updated.




 builtin/log.c | 21 ++++++++++++++-------
 log-tree.c    |  2 +-
 log-tree.h    |  1 -
 revision.h    |  1 +
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git c/builtin/log.c w/builtin/log.c
index 0a7ed4bef9..d938fba27e 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -37,6 +37,8 @@
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
+#define FORMAT_PATCH_NAME_MIN 8
+#define FORMAT_PATCH_NAME_MAX 64
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -50,6 +52,7 @@ static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX;
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
@@ -150,6 +153,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->patch_name_max = fmt_patch_name_max;
 	rev->show_signature = default_show_signature;
 	rev->encode_email_headers = default_encode_email_headers;
 	rev->diffopt.flags.allow_textconv = 1;
@@ -454,6 +458,12 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
 		return git_config_string(&fmt_patch_subject_prefix, var, value);
+	if (!strcmp(var, "format.patchnamemax")) {
+		fmt_patch_name_max = git_config_int(var, value);
+		if (fmt_patch_name_max < FORMAT_PATCH_NAME_MIN)
+			fmt_patch_name_max = FORMAT_PATCH_NAME_MIN;
+		return 0;
+	}
 	if (!strcmp(var, "format.encodeemailheaders")) {
 		default_encode_email_headers = git_config_bool(var, value);
 		return 0;
@@ -955,15 +965,9 @@ static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
-	int suffix_len = strlen(rev->patch_suffix) + 1;
 
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
-		if (filename.len >=
-		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {
-			strbuf_release(&filename);
-			return error(_("name of output directory is too long"));
-		}
 		strbuf_complete(&filename, '/');
 	}
 
@@ -1751,6 +1755,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    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_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
+			    N_("max length of output filename")),
 		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
 			    N_("Use [RFC PATCH] instead of [PATCH]"),
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback),
@@ -1822,6 +1828,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	init_display_notes(&notes_opt);
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	rev.subject_prefix = fmt_patch_subject_prefix;
 	rev.show_notes = show_notes;
 	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -1831,7 +1838,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
-	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
@@ -1935,6 +1941,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
 	rev.zero_commit = zero_commit;
+	rev.patch_name_max = fmt_patch_name_max;
 
 	if (!rev.diffopt.flags.text && !no_binary_diff)
 		rev.diffopt.flags.binary = 1;
diff --git c/log-tree.c w/log-tree.c
index 1927f917ce..fd0dde97ec 100644
--- c/log-tree.c
+++ w/log-tree.c
@@ -367,7 +367,7 @@ void fmt_output_subject(struct strbuf *filename,
 	const char *suffix = info->patch_suffix;
 	int nr = info->nr;
 	int start_len = filename->len;
-	int max_len = start_len + FORMAT_PATCH_NAME_MAX - (strlen(suffix) + 1);
+	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);
diff --git c/log-tree.h w/log-tree.h
index 8fa79289ec..1e8c91dbf2 100644
--- c/log-tree.h
+++ w/log-tree.h
@@ -33,7 +33,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
 
-#define FORMAT_PATCH_NAME_MAX 64
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
diff --git c/revision.h w/revision.h
index f6bf860d19..086ff10280 100644
--- c/revision.h
+++ w/revision.h
@@ -238,6 +238,7 @@ struct rev_info {
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
+	int		patch_name_max;
 	int		no_inline;
 	int		show_log_size;
 	struct string_list *mailmap;

[PATCH] format-patch: make output filename configurable

[Junio C Hamano]

For the past 15 years, we've used the hardcoded 64 as the length
limit of the filename of the output from the "git format-patch"
command.  Since the value is shorter than the 80-column terminal, it
could grow without line wrapping a bit.  At the same time, since the
value is longer than half of the 80-column terminal, we could fit
two or more of them in "ls" output on such a terminal if we allowed
to lower it.

Introduce a new command line option --filename-max-length=<n> and a
new configuration variable format.filenameMaxLength to override the
hardcoded default.

While we are at it, remove a check that the name of output directory
does not exceed PATH_MAX---this check is pointless in that by the
time control reaches the function, the caller would already have
done an equivalent of "mkdir -p", so if the system does not like an
overly long directory name, the control wouldn't have reached here,
and otherwise, we know that the system allowed the output directory
to exist.  In the worst case, we will get an error when we try to
open the output file and handle the error correctly anyway.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Unlike the preview I sent out earlier, this is with doc/test,
   with consistent naming between option and config and adjust the
   minimum a bit better.

 Documentation/git-format-patch.txt |  8 +++++
 builtin/log.c                      | 22 ++++++++----
 log-tree.c                         |  2 +-
 log-tree.h                         |  1 -
 revision.h                         |  1 +
 t/t4014-format-patch.sh            | 54 ++++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 0f81d0437b..6c0da53d1b 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -28,6 +28,7 @@ SYNOPSIS
 		   [--no-notes | --notes[=<ref>]]
 		   [--interdiff=<previous>]
 		   [--range-diff=<previous> [--creation-factor=<percent>]]
+		   [--filename-max-length=<n>]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -200,6 +201,13 @@ populated with placeholder text.
 	allows for useful naming of a patch series, and can be
 	combined with the `--numbered` option.
 
+--filename-max-length=<n>::
+	Instead of the standard 64 bytes, chomp the generated output
+	filenames at around '<n>' bytes (too short a value will be
+	silently raised to a reasonable length).  Defaults to the
+	value of the `format.filenamemaxlength` configuration
+	variable, or 64 if unconfigured.
+
 --rfc::
 	Alias for `--subject-prefix="RFC PATCH"`. RFC means "Request For
 	Comments"; use this when sending an experimental patch for
diff --git a/builtin/log.c b/builtin/log.c
index 0a7ed4bef9..140a7feedc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -37,6 +37,7 @@
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
+#define FORMAT_PATCH_NAME_MAX 64
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -50,6 +51,7 @@ static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX;
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
@@ -150,6 +152,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->patch_name_max = fmt_patch_name_max;
 	rev->show_signature = default_show_signature;
 	rev->encode_email_headers = default_encode_email_headers;
 	rev->diffopt.flags.allow_textconv = 1;
@@ -454,6 +457,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
 		return git_config_string(&fmt_patch_subject_prefix, var, value);
+	if (!strcmp(var, "format.filenamemaxlength")) {
+		fmt_patch_name_max = git_config_int(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "format.encodeemailheaders")) {
 		default_encode_email_headers = git_config_bool(var, value);
 		return 0;
@@ -955,15 +962,9 @@ static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
-	int suffix_len = strlen(rev->patch_suffix) + 1;
 
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
-		if (filename.len >=
-		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {
-			strbuf_release(&filename);
-			return error(_("name of output directory is too long"));
-		}
 		strbuf_complete(&filename, '/');
 	}
 
@@ -1751,6 +1752,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    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_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
+			    N_("max length of output filename")),
 		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
 			    N_("Use [RFC PATCH] instead of [PATCH]"),
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback),
@@ -1822,6 +1825,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	init_display_notes(&notes_opt);
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	rev.subject_prefix = fmt_patch_subject_prefix;
 	rev.show_notes = show_notes;
 	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -1831,7 +1835,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
-	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
@@ -1851,6 +1854,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
+	if (fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
+		fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix);
+
 	if (cover_from_description_arg)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
@@ -1935,6 +1942,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
 	rev.zero_commit = zero_commit;
+	rev.patch_name_max = fmt_patch_name_max;
 
 	if (!rev.diffopt.flags.text && !no_binary_diff)
 		rev.diffopt.flags.binary = 1;
diff --git a/log-tree.c b/log-tree.c
index 1927f917ce..fd0dde97ec 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -367,7 +367,7 @@ void fmt_output_subject(struct strbuf *filename,
 	const char *suffix = info->patch_suffix;
 	int nr = info->nr;
 	int start_len = filename->len;
-	int max_len = start_len + FORMAT_PATCH_NAME_MAX - (strlen(suffix) + 1);
+	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);
diff --git a/log-tree.h b/log-tree.h
index 8fa79289ec..1e8c91dbf2 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -33,7 +33,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
 
-#define FORMAT_PATCH_NAME_MAX 64
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
diff --git a/revision.h b/revision.h
index f6bf860d19..086ff10280 100644
--- a/revision.h
+++ b/revision.h
@@ -238,6 +238,7 @@ struct rev_info {
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
+	int		patch_name_max;
 	int		no_inline;
 	int		show_log_size;
 	struct string_list *mailmap;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 294e76c860..024c0a026d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -313,6 +313,60 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
+test_expect_success 'filename length limit' '
+	test_when_finished "rm -f 000*" &&
+	rm -rf 000[1-9]-*.patch &&
+	for len in 15 25 35
+	do
+		git format-patch --filename-max-length=$len -3 side &&
+		max=$(
+			for patch in 000[1-9]-*.patch
+			do
+				echo "$patch" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
+test_expect_success 'filename length limit from config' '
+	test_when_finished "rm -f 000*" &&
+	rm -rf 000[1-9]-*.patch &&
+	for len in 15 25 35
+	do
+		git -c format.filenameMaxLength=$len format-patch -3 side &&
+		max=$(
+			for patch in 000[1-9]-*.patch
+			do
+				echo "$patch" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
+test_expect_success 'filename limit applies only to basename' '
+	test_when_finished "rm -rf patches/" &&
+	rm -rf patches/ &&
+	for len in 15 25 35
+	do
+		git format-patch -o patches --filename-max-length=$len -3 side &&
+		max=$(
+			for patch in patches/000[1-9]-*.patch
+			do
+				echo "${patch#patches/}" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
 test_expect_success 'reroll count' '
 	rm -fr patches &&
 	git format-patch -o patches --cover-letter --reroll-count 4 master..side >list &&
-- 
2.29.2-306-gddee074d3d

Re: [PATCH] format-patch: make output filename configurable

[Eric Sunshine]

On Fri, Nov 6, 2020 at 4:56 PM Junio C Hamano <gitster@pobox.com> wrote:
> [...]
> Introduce a new command line option --filename-max-length=<n> and a
> new configuration variable format.filenameMaxLength to override the
> hardcoded default.
> [...]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -200,6 +201,13 @@ populated with placeholder text.
> +--filename-max-length=<n>::
> +       Instead of the standard 64 bytes, chomp the generated output
> +       filenames at around '<n>' bytes (too short a value will be
> +       silently raised to a reasonable length).  Defaults to the
> +       value of the `format.filenamemaxlength` configuration
> +       variable, or 64 if unconfigured.

In user-facing documentation, I believe practice is to camelCase the
configuration variable name, so:
s/filenamemaxlength/filenameMaxLength/

This new configuration also ought to be mentioned in
Documentation/config/format.txt.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -37,6 +37,7 @@
> +#define FORMAT_PATCH_NAME_MAX 64

Nit: A slightly less confusing name would be FORMAT_PATCH_NAME_MAX_DEFAULT.

[PATCH v2] format-patch: make output filename configurable

[Junio C Hamano]

For the past 15 years, we've used the hardcoded 64 as the length
limit of the filename of the output from the "git format-patch"
command.  Since the value is shorter than the 80-column terminal, it
could grow without line wrapping a bit.  At the same time, since the
value is longer than half of the 80-column terminal, we could fit
two or more of them in "ls" output on such a terminal if we allowed
to lower it.

Introduce a new command line option --filename-max-length=<n> and a
new configuration variable format.filenameMaxLength to override the
hardcoded default.

While we are at it, remove a check that the name of output directory
does not exceed PATH_MAX---this check is pointless in that by the
time control reaches the function, the caller would already have
done an equivalent of "mkdir -p", so if the system does not like an
overly long directory name, the control wouldn't have reached here,
and otherwise, we know that the system allowed the output directory
to exist.  In the worst case, we will get an error when we try to
open the output file and handle the error correctly anyway.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff against v1:
1:  1b012b4164 ! 1:  d1d91e4833 format-patch: make output filename configurable
    @@ Commit message
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    + ## Documentation/config/format.txt ##
    +@@ Documentation/config/format.txt: format.outputDirectory::
    + 	Set a custom directory to store the resulting files instead of the
    + 	current working directory. All directory components will be created.
    + 
    ++format.filenameMaxLength::
    ++	The maximum length of the output filenames generated by the
    ++	`format-patch` command; defaults to 64.  Can be overridden
    ++	by the `--filename-max-length=<n>` command line option.
    ++
    + format.useAutoBase::
    + 	A boolean value which lets you enable the `--base=auto` option of
    + 	format-patch by default. Can also be set to "whenAble" to allow
    +
      ## Documentation/git-format-patch.txt ##
     @@ Documentation/git-format-patch.txt: SYNOPSIS
      		   [--no-notes | --notes[=<ref>]]
    @@ Documentation/git-format-patch.txt: populated with placeholder text.
     +	Instead of the standard 64 bytes, chomp the generated output
     +	filenames at around '<n>' bytes (too short a value will be
     +	silently raised to a reasonable length).  Defaults to the
    -+	value of the `format.filenamemaxlength` configuration
    ++	value of the `format.filenameMaxLength` configuration
     +	variable, or 64 if unconfigured.
     +
      --rfc::
    @@ builtin/log.c
      
      #define MAIL_DEFAULT_WRAP 72
      #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
    -+#define FORMAT_PATCH_NAME_MAX 64
    ++#define FORMAT_PATCH_NAME_MAX_DEFAULT 64
      
      /* Set a default date-time format for git log ("log.date" config variable) */
      static const char *default_date_mode = NULL;
    @@ builtin/log.c: static int decoration_style;
      static int decoration_given;
      static int use_mailmap_config = 1;
      static const char *fmt_patch_subject_prefix = "PATCH";
    -+static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX;
    ++static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
      static const char *fmt_pretty;
      
      static const char * const builtin_log_usage[] = {

 Documentation/config/format.txt    |  5 +++
 Documentation/git-format-patch.txt |  8 +++++
 builtin/log.c                      | 22 ++++++++----
 log-tree.c                         |  2 +-
 log-tree.h                         |  1 -
 revision.h                         |  1 +
 t/t4014-format-patch.sh            | 54 ++++++++++++++++++++++++++++++
 7 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index c2efd8758a..7f6d11b5d5 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -94,6 +94,11 @@ format.outputDirectory::
 	Set a custom directory to store the resulting files instead of the
 	current working directory. All directory components will be created.
 
+format.filenameMaxLength::
+	The maximum length of the output filenames generated by the
+	`format-patch` command; defaults to 64.  Can be overridden
+	by the `--filename-max-length=<n>` command line option.
+
 format.useAutoBase::
 	A boolean value which lets you enable the `--base=auto` option of
 	format-patch by default. Can also be set to "whenAble" to allow
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 0f81d0437b..3347702b71 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -28,6 +28,7 @@ SYNOPSIS
 		   [--no-notes | --notes[=<ref>]]
 		   [--interdiff=<previous>]
 		   [--range-diff=<previous> [--creation-factor=<percent>]]
+		   [--filename-max-length=<n>]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -200,6 +201,13 @@ populated with placeholder text.
 	allows for useful naming of a patch series, and can be
 	combined with the `--numbered` option.
 
+--filename-max-length=<n>::
+	Instead of the standard 64 bytes, chomp the generated output
+	filenames at around '<n>' bytes (too short a value will be
+	silently raised to a reasonable length).  Defaults to the
+	value of the `format.filenameMaxLength` configuration
+	variable, or 64 if unconfigured.
+
 --rfc::
 	Alias for `--subject-prefix="RFC PATCH"`. RFC means "Request For
 	Comments"; use this when sending an experimental patch for
diff --git a/builtin/log.c b/builtin/log.c
index 0a7ed4bef9..861ac17da0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -37,6 +37,7 @@
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
+#define FORMAT_PATCH_NAME_MAX_DEFAULT 64
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -50,6 +51,7 @@ static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
@@ -150,6 +152,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->patch_name_max = fmt_patch_name_max;
 	rev->show_signature = default_show_signature;
 	rev->encode_email_headers = default_encode_email_headers;
 	rev->diffopt.flags.allow_textconv = 1;
@@ -454,6 +457,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
 		return git_config_string(&fmt_patch_subject_prefix, var, value);
+	if (!strcmp(var, "format.filenamemaxlength")) {
+		fmt_patch_name_max = git_config_int(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "format.encodeemailheaders")) {
 		default_encode_email_headers = git_config_bool(var, value);
 		return 0;
@@ -955,15 +962,9 @@ static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
-	int suffix_len = strlen(rev->patch_suffix) + 1;
 
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
-		if (filename.len >=
-		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {
-			strbuf_release(&filename);
-			return error(_("name of output directory is too long"));
-		}
 		strbuf_complete(&filename, '/');
 	}
 
@@ -1751,6 +1752,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    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_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
+			    N_("max length of output filename")),
 		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
 			    N_("Use [RFC PATCH] instead of [PATCH]"),
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback),
@@ -1822,6 +1825,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	init_display_notes(&notes_opt);
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	rev.subject_prefix = fmt_patch_subject_prefix;
 	rev.show_notes = show_notes;
 	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -1831,7 +1835,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
-	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
@@ -1851,6 +1854,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
+	if (fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
+		fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix);
+
 	if (cover_from_description_arg)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
@@ -1935,6 +1942,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
 	rev.zero_commit = zero_commit;
+	rev.patch_name_max = fmt_patch_name_max;
 
 	if (!rev.diffopt.flags.text && !no_binary_diff)
 		rev.diffopt.flags.binary = 1;
diff --git a/log-tree.c b/log-tree.c
index 1927f917ce..fd0dde97ec 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -367,7 +367,7 @@ void fmt_output_subject(struct strbuf *filename,
 	const char *suffix = info->patch_suffix;
 	int nr = info->nr;
 	int start_len = filename->len;
-	int max_len = start_len + FORMAT_PATCH_NAME_MAX - (strlen(suffix) + 1);
+	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);
diff --git a/log-tree.h b/log-tree.h
index 8fa79289ec..1e8c91dbf2 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -33,7 +33,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
 
-#define FORMAT_PATCH_NAME_MAX 64
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
diff --git a/revision.h b/revision.h
index f6bf860d19..086ff10280 100644
--- a/revision.h
+++ b/revision.h
@@ -238,6 +238,7 @@ struct rev_info {
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
+	int		patch_name_max;
 	int		no_inline;
 	int		show_log_size;
 	struct string_list *mailmap;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 294e76c860..024c0a026d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -313,6 +313,60 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
+test_expect_success 'filename length limit' '
+	test_when_finished "rm -f 000*" &&
+	rm -rf 000[1-9]-*.patch &&
+	for len in 15 25 35
+	do
+		git format-patch --filename-max-length=$len -3 side &&
+		max=$(
+			for patch in 000[1-9]-*.patch
+			do
+				echo "$patch" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
+test_expect_success 'filename length limit from config' '
+	test_when_finished "rm -f 000*" &&
+	rm -rf 000[1-9]-*.patch &&
+	for len in 15 25 35
+	do
+		git -c format.filenameMaxLength=$len format-patch -3 side &&
+		max=$(
+			for patch in 000[1-9]-*.patch
+			do
+				echo "$patch" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
+test_expect_success 'filename limit applies only to basename' '
+	test_when_finished "rm -rf patches/" &&
+	rm -rf patches/ &&
+	for len in 15 25 35
+	do
+		git format-patch -o patches --filename-max-length=$len -3 side &&
+		max=$(
+			for patch in patches/000[1-9]-*.patch
+			do
+				echo "${patch#patches/}" | wc -c
+			done |
+			sort -nr |
+			head -n 1
+		) &&
+		test $max -le $len || return 1
+	done
+'
+
 test_expect_success 'reroll count' '
 	rm -fr patches &&
 	git format-patch -o patches --cover-letter --reroll-count 4 master..side >list &&
-- 
2.29.2-306-gddee074d3d

Re: [PATCH v2] format-patch: make output filename configurable

[Jeff King]

On Mon, Nov 09, 2020 at 11:23:48AM -0800, Junio C Hamano wrote:

> @@ -1822,6 +1825,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	init_display_notes(&notes_opt);
>  	git_config(git_format_config, NULL);
>  	repo_init_revisions(the_repository, &rev, prefix);
> +	rev.subject_prefix = fmt_patch_subject_prefix;
>  	rev.show_notes = show_notes;
>  	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
>  	rev.commit_format = CMIT_FMT_EMAIL;
> @@ -1831,7 +1835,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.diff = 1;
>  	rev.max_parents = 1;
>  	rev.diffopt.flags.recursive = 1;
> -	rev.subject_prefix = fmt_patch_subject_prefix;
>  	memset(&s_r_opt, 0, sizeof(s_r_opt));
>  	s_r_opt.def = "HEAD";
>  	s_r_opt.revarg_opt = REVARG_COMMITTISH;

It's not clear to me what these hunks are doing. I'm trying really hard
to find some subtle reason that we need to init this field sooner, but I
can't. It really looks like it might be leftover noise.

-Peff

Re: [PATCH v2] format-patch: make output filename configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 09, 2020 at 11:23:48AM -0800, Junio C Hamano wrote:
>
>> @@ -1822,6 +1825,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	init_display_notes(&notes_opt);
>>  	git_config(git_format_config, NULL);
>>  	repo_init_revisions(the_repository, &rev, prefix);
>> +	rev.subject_prefix = fmt_patch_subject_prefix;
>>  	rev.show_notes = show_notes;
>>  	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
>>  	rev.commit_format = CMIT_FMT_EMAIL;
>> @@ -1831,7 +1835,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	rev.diff = 1;
>>  	rev.max_parents = 1;
>>  	rev.diffopt.flags.recursive = 1;
>> -	rev.subject_prefix = fmt_patch_subject_prefix;
>>  	memset(&s_r_opt, 0, sizeof(s_r_opt));
>>  	s_r_opt.def = "HEAD";
>>  	s_r_opt.revarg_opt = REVARG_COMMITTISH;
>
> It's not clear to me what these hunks are doing. I'm trying really hard
> to find some subtle reason that we need to init this field sooner, but I
> can't. It really looks like it might be leftover noise.

It indeed was leftover noise and has nothing to do with the output
filename length configurablility (I suspect it is a true no-op).

Thanks.

RE: [PATCH v2] format-patch: make output filename configurable

[hukeping]

>For the past 15 years, we've used the hardcoded 64 as the length limit of the
>filename of the output from the "git format-patch"
>command.  Since the value is shorter than the 80-column terminal, it could grow
>without line wrapping a bit.  At the same time, since the value is longer than half
>of the 80-column terminal, we could fit two or more of them in "ls" output on
>such a terminal if we allowed to lower it.
>
>Introduce a new command line option --filename-max-length=<n> and a new
>configuration variable format.filenameMaxLength to override the hardcoded
>default.
>
It would be very hard to remove a config knob rather than add  a new one and we already
have too many.

Does it worth to add a new configuration variable for this or just a hard-coded value is enough?

>While we are at it, remove a check that the name of output directory does not
>exceed PATH_MAX---this check is pointless in that by the time control reaches the
>function, the caller would already have done an equivalent of "mkdir -p", so if the
>system does not like an overly long directory name, the control wouldn't have
>reached here, and otherwise, we know that the system allowed the output
>directory to exist.  In the worst case, we will get an error when we try to open the
>output file and handle the error correctly anyway.
>
>Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v2] format-patch: make output filename configurable

[Junio C Hamano]

hukeping <hukeping@huawei.com> writes:

> It would be very hard to remove a config knob rather than add a
> new one and we already have too many.
>
> Does it worth to add a new configuration variable for this or just
> a hard-coded value is enough?

I personally would say "yes, the current code that limits to 64 is
enough", but you, as the person who said that you do not like the
current hard-coded value, are not in the position to ask that
question, I would have to say---if it were enough for you, you
wouldn't have complained about 64 in the first place ;-)

RE: [PATCH v2] format-patch: make output filename configurable

[hukeping]

>> It would be very hard to remove a config knob rather than add a new
>> one and we already have too many.
>>
>> Does it worth to add a new configuration variable for this or just a
>> hard-coded value is enough?
>
>I personally would say "yes, the current code that limits to 64 is enough", but you,
>as the person who said that you do not like the current hard-coded value, are not
>in the position to ask that question, I would have to say---if it were enough for
>you, you wouldn't have complained about 64 in the first place ;-)

The original motivation is to lengthening the limit because of file name truncated problem,
so update the value to a larger one seems like the simplest way for me.

The v2 patch can fundamentally solve this problem,  just a little worry about the more and
more git-config knobs.

Re: [PATCH v2] format-patch: make output filename configurable

[Junio C Hamano]

hukeping <hukeping@huawei.com> writes:

>>> Does it worth to add a new configuration variable for this or just a
>>> hard-coded value is enough?
>>
>>I personally would say "yes, the current code that limits to 64 is enough", but you,
>>as the person who said that you do not like the current hard-coded value, are not
>>in the position to ask that question, I would have to say---if it were enough for
>>you, you wouldn't have complained about 64 in the first place ;-)
>
> The original motivation is to lengthening the limit because of
> file name truncated problem, so update the value to a larger one
> seems like the simplest way for me.

It actually would be an improvement to me if we shorten the current
hardcoded limit.  In fact, 64 is still a bit too long to fit in
dired buffer and raising the limit will make it even worse.

In other words, you are forgetting that a larger limit is not always
a better limit.

I am not saying that between my wish to keep 64 as near optimal (and
possibly make it shorter) and your wish to make it longer to avoid
truncation, the former wish is more important than the latter.  The
former however is at least as important as the latter [*1*].

And once you realize that one hardcoded limit never is ideal for
everybody, you wouldn't even dream to suggest that raising the limit
to another hardcoded value is better. Adding a configuration knob is
a way to treat people with respect and give them choice to suit the
system to their taste.

The names of output files from format-patch is a very local matter,
because once it is fed to "git send-email", the receiving end does
not even care how many letters you used for your filename---after
all, you may not have used an intermediate file at all.  It is a
good place to give personal choice, as opposed to parts of the
system where interaction among multiple people happens (e.g. we
wouldn't dream of making the characters allowed in refnames
configurable---that would cause chaos between hosting sites and
fetchers), where we give more careful thought before making things
configurable in order to avoid fracturing the ecosystem.

Thanks.


[Footnote]

*1* ... exactly because we know that most users who used Git in the
past 15 years were happy with the current limit as we haven't seen
much complaint until your patch.  We cannot tell if they wanted the
limit to be shorter, or longer, though.