mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Jeff King <>
Cc: Hu Keping <>,,,,,,
Subject: Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80
Date: Thu, 05 Nov 2020 13:16:53 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Jeff King's message of "Thu, 5 Nov 2020 10:01:49 -0500")

Jeff King <> writes:

>> Considered the prefix patch number "0001-" would take 5 characters, increase
> 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.

  reply	other threads:[~2020-11-05 21:16 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 [this message]
2020-11-06  8:51     ` hukeping
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).