Re: [PATCH v2] format-patch: add --description-file option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 11, 2023 at 02:38:05PM -0700, Junio C Hamano wrote:
+	if (strbuf_read_file(buf, desc_file, 2000) < 0)

You would probably want to do "2000" -> "0" here.

hmm, yeah, i wonder where i got it from, given that there is no precedent. i suppose i simply thought that 2k is a reasonably expectable max size for a description. if you think the default 8k hint is a better idea, then let's go with it.

 static void prepare_cover_text(struct pretty_print_context *pp,
+			       const char *description_file,
 			       const char *branch_name,

This is kind of suboptimal, but let's let it pass.

A better design is to pass the description string itself to this
function and the make_cover_letter() function, and have the higher
level callers of the callchain prepare the either read_desc_file()
or read_branch_desc() to prepare that string before calling into the
callchain.
there is only one caller, and doing this change would essentially result in inlining prepare_cover_text(). probably not the best outcome.

Such a division of labor between the callers and this
function will allow us to more easily add another option to the
command, to feed the description string itself (instead of having to
create a temporary file and storing the description in it).

that's a good point. in fact, passing in the description directly would probably fit my use case better ... i just happened to already have the code for creating that temp file anyway (for editing), so i didn't give it a second thought. i can add both options in the same go, given that it's almost no code.

regards



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux