Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80

From: Jeff King <peff@peff.net>
To: Hu Keping <hukeping@huawei.com>
Cc: git@vger.kernel.org, zhengjunling@huawei.com,
	zhuangbiaowei@huawei.com, git@stormcloud9.net,
	rafa.almas@gmail.com, l.s.r@web.de, gitster@pobox.com
Subject: Re: [PATCH] Lengthening FORMAT_PATCH_NAME_MAX to 80
Date: Thu, 5 Nov 2020 10:01:49 -0500	[thread overview]
Message-ID: <20201105150149.GA107127@coredump.intra.peff.net> (raw)
In-Reply-To: <20201105201548.2333425-1-hukeping@huawei.com>

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