Re: [PATCH] builtin/log.c: prepend "RFC" on --rfc
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 28, 2023, 16:30 UTC
- 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:
Show 15 quoted lines
> 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
Show 5 quoted lines
> for anybody expecting: > > git format-patch --subject=foo --rfc > > to override the --subject line, but that seems rather unlikely.
Show 69 quoted lines
>> 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.