From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Fri, 11 Aug 2023 22:29:41 GMT
Subject: Re: [PATCH v2] format-patch: add --description-file option
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.

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