mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: hukeping <>
To: Junio C Hamano <>, Jeff King <>
Cc: "" <>,
	"Zhengjunling (JRing, Task Force)" <>,
	zhuangbiaowei <>,
	"" <>,
	"" <>,
	"" <>
Subject: RE: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80
Date: Fri, 6 Nov 2020 08:51:32 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

>-----Original Message-----
>From: Junio C Hamano []
>Sent: Friday, November 6, 2020 5:17 AM
>To: Jeff King <>
>Cc: hukeping <>;; Zhengjunling (JRing,
>Task Force) <>; zhuangbiaowei
>Subject: Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80
>Jeff King <> 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?


  reply	other threads:[~2020-11-06  8:51 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-05 20:15 [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80 Hu Keping
2020-11-05 15:01 ` Jeff King
2020-11-05 21:16   ` Junio C Hamano
2020-11-06  8:51     ` hukeping [this message]
2020-11-06 17:45       ` Junio C Hamano
2020-11-06 20:50     ` Junio C Hamano
2020-11-06 21:56       ` [PATCH] format-patch: make output filename configurable Junio C Hamano
2020-11-06 22:05         ` Eric Sunshine
2020-11-09 19:23           ` [PATCH v2] " Junio C Hamano
2020-11-10  0:23             ` Jeff King
2020-11-10  1:43               ` Junio C Hamano
2020-11-10  2:31             ` hukeping
2020-11-10  2:37               ` Junio C Hamano
2020-11-10  4:44                 ` hukeping
2020-11-10  5:40                   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).