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

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Aug 11, 2023, 22:29 UTC
Message-ID: <ZNa2VQQzZmaMzM4C@ugly>
In-Reply-To: <xmqq1qg9qmyq.fsf@gitster.g>

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.

Show 12 quoted lines

>>  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.

Show 5 quoted lines

>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