| Message ID | 20201105201548.2333425-1-hukeping@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lengthening FORMAT_PATCH_NAME_MAX to 80 | expand |
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
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.
>-----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.
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.
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(¬es_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, ¬es_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;
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 *);