From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 28 Aug 2023 16:30:36 GMT
Subject: Re: [PATCH] builtin/log.c: prepend "RFC" on --rfc
Message-ID: <ae22b71b-73ea-4634-bd2a-4b64082be955@gmail.com>
In-Reply-To: <20230828144215.GA2537587@coredump.intra.peff.net>

On 28/08/2023 15:42, Jeff King wrote:
> On Mon, Aug 28, 2023 at 02:50:34PM +0200, Drew DeVault wrote:
> 
>> Rather than replacing the configured subject prefix (either through the
>> git config or command line) entirely with "RFC PATCH", this change
>> prepends RFC to whatever subject prefix was already in use.
>>
>> This is useful, for example, when a user is working on a repository that
>> has a subject prefix considered to disambiguate patches:
>>
>> 	git config format.subjectPrefix 'PATCH my-project'
>>
>> Prior to this change, formatting patches with --rfc would lose the
>> 'my-project' information.
> 
> This sounds like a good change to me. 

I agree it sounds like a good change but if we're going to change it 
than I think we should ensure

     git format-patch --subject-prefix=foo --rfc

and

     git format-patch --rfc --subject-prefix=foo

give the same result. That would mean dropping rfc_callback() and using 
OPT_BOOL() instead of OPT_CALLBACK_F(). We could add the "RFC " prefix 
just before we add the re-roll suffix.

Best Wishes

Phillip

It would be backwards-incompatible
> for anybody expecting:
> 
>    git format-patch --subject=foo --rfc
> 
> to override the --subject line, but that seems rather unlikely.

>> Implementation note: this introduces a small memory leak, but freeing it
>> requires a non-trivial amount of refactoring and some dubious choices
>> that I was not sure of for a small patch; and it seems like memory leaks
>> in this context are tolerated anyway from a perusal of the existing
>> code.
> 
> We do have a lot of small leaks like this, but we've been trying to
> clean them up slowly. There's some infrastructure in the test suite for
> marking scripts as leak-free, but t4014 is not yet there, so this
> won't cause CI to complain at this point.
> 
> It is tempting while we are here and thinking about it to put in an easy
> hack, like storing the allocated string in a static variable.
> 
>>   static int rfc_callback(const struct option *opt, const char *arg, int unset)
>>   {
>> +	int n;
>> +	char *prefix;
>> +	const char *prev;
>> +
>>   	BUG_ON_OPT_NEG(unset);
>>   	BUG_ON_OPT_ARG(arg);
>> -	return subject_prefix_callback(opt, "RFC PATCH", unset);
>> +
>> +	prev = ((struct rev_info *)opt->value)->subject_prefix;
>> +	assert(prev != NULL);
>> +	n = snprintf(NULL, 0, "RFC %s", prev);
>> +	assert(n > 0);
>> +	prefix = xmalloc(n + 1);
>> +	n = snprintf(prefix, n + 1, "RFC %s", prev);
>> +	assert(n > 0);
>> +
>> +	return subject_prefix_callback(opt, prefix, unset);
>>   }
> 
> We try to avoid manually computing string sizes like this, since it's
> error-prone and can be subject to integer overflow attacks (not in this
> case, but every instance makes auditing harder). You can use xstrfmt()
> instead.
> 
> Coupled with the leak-hack from above, maybe just:
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index db3a88bfe9..579c3a2419 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1476,9 +1476,19 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,
>   
>   static int rfc_callback(const struct option *opt, const char *arg, int unset)
>   {
> +	/*
> +	 * "avoid" leak by holding on to a reference to the memory, since we
> +	 * need the string for the lifetime of the process anyway
> +	 */
> +	static char *prefix;
> +
>   	BUG_ON_OPT_NEG(unset);
>   	BUG_ON_OPT_ARG(arg);
> -	return subject_prefix_callback(opt, "RFC PATCH", unset);
> +
> +	free(prefix);
> +	prefix = xstrfmt("RFC %s", ((struct rev_info *)opt->value)->subject_prefix);
> +
> +	return subject_prefix_callback(opt, prefix, unset);
>   }
>   
>   static int numbered_cmdline_opt = 0;
> 
> The rest of the patch (docs and tests) looked good to me.