| Message ID | xmqqwnyubagr.fsf@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] format-patch: make output filename configurable | expand |
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(¬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 +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
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(¬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 +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.
>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>
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 ;-)
>> 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.
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.
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(¬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 +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 &&