From: Andrzej Hunt <andrzej@ahunt.org>
To: Jeff King <peff@peff.net>,
Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Andrzej Hunt <ajrhunt@google.com>
Subject: Re: [PATCH 7/7] parse-options: don't leak alias help messages
Date: Sun, 14 Mar 2021 18:03:22 +0100 [thread overview]
Message-ID: <6ddb4ca3-8486-a8cf-24ed-1ae6220602f3@ahunt.org> (raw)
In-Reply-To: <YEZ/BWWbpfVwl6nO@coredump.intra.peff.net>
On 08/03/2021 20:46, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:
>
>> +static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)
>
> A few style nits:
>
> - omit the space between "**" and preprocessed_options
>
> - we'd usually break a long line (after the first parameter comma)
>
> I think preprocessed_options shouldn't be const here. After all, our aim
> is to free it! I'm also not sure why it's a pointer-to-pointer. If we
> were setting it to NULL after freeing, that would be valuable, but we
> don't. So all together >
> static void free_preprocessed_options(struct option *preprocessed_options,
> const struct option *original_options)
I'm not sure what I was originally thinking when I used the
pointer-to-pointer - I've incorporated your suggestions, they do make
everything easier to read. Moreover we'll remove original_options as per
your later suggestions anyway.
>
>> + for (i = 0; original_options[i].type != OPTION_END; i++) {
>> + if (original_options[i].type == OPTION_ALIAS) {
>> + free((void *)(*preprocessed_options)[i].help);
>> + }
>> + }
>
> OK, so we look through the original options to find ones that became an
> alias, and then free them. Makes sense.
>
> Do the indexes always correspond between the original and the
> preprocessed arrays? I _think_ so, but preprocess_options() is a little
> hard to follow.
>
> If the preprocess code set a flag in the resulting option, though, we
> could make it much more obviously correct. And avoid having to pass
> original_options at all.
_At this time_, indexes always correspond between the original and
preprocessed options, but in the back of my mind I was still a little
bit uncomfortable depending on that. Your suggestion is much better - so
I've gone ahead and implemented it.
>
>> + free((void *)*preprocessed_options);
>
> With the interface suggestions above, this becomes just:
>
> free(preprocessed_options);
>
>> @@ -838,15 +855,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
>> int flags)
>> {
>> struct parse_opt_ctx_t ctx;
>> - struct option *real_options;
>> + const struct option *preprocessed_options, *original_options = NULL;
>>
>> disallow_abbreviated_options =
>> git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
>>
>> memset(&ctx, 0, sizeof(ctx));
>> - real_options = preprocess_options(&ctx, options);
>> - if (real_options)
>> - options = real_options;
>> + preprocessed_options = preprocess_options(&ctx, options);
>> + if (preprocessed_options) {
>> + original_options = options;
>> + options = preprocessed_options;
>> + }
>
> OK, we have to keep two variables now rather than aliasing "options",
> because we need the original for feeding to the free function (but this
> hunk too would go away if we set a flag).
Indeed - after adding the flag as suggested, the changes to
parse_options() are reduced down to calling free_preprocessed_options()
instead of free() - which is quite a nice simplification.
>
> To spell it out, I mean something like on the writing side:
>
> diff --git a/parse-options.c b/parse-options.c
> index fbea16eaf5..43431b96b1 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -678,6 +678,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
> newopt[i].short_name = short_name;
> newopt[i].long_name = long_name;
> newopt[i].help = strbuf_detach(&help, NULL);
> + newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
> break;
> }
>
> diff --git a/parse-options.h b/parse-options.h
> index ff6506a504..32b0b49a2d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -47,7 +47,8 @@ enum parse_opt_option_flags {
> PARSE_OPT_SHELL_EVAL = 256,
> PARSE_OPT_NOCOMPLETE = 512,
> PARSE_OPT_COMP_ARG = 1024,
> - PARSE_OPT_CMDMODE = 2048
> + PARSE_OPT_CMDMODE = 2048,
> + PARSE_OPT_FROM_ALIAS = 4096,
> };
>
> enum parse_opt_result {
>
> (as an aside, these manual bitfield values are tedious; I wouldn't be
> sad to see them converted to "1 << 0", "1 << 1", and so on).
I've added a separate patch to take care of the bitfield improvements -
and have incorporated the PARSE_OPT_FROM_ALIAS change into the original
patch!
next prev parent reply other threads:[~2021-03-14 17:04 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-08 18:36 [PATCH 0/7] Fix all leaks in t0001 Andrzej Hunt via GitGitGadget
2021-03-08 18:36 ` [PATCH 1/7] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-08 19:01 ` Jeff King
2021-03-14 18:07 ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 2/7] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-08 19:03 ` Jeff King
2021-03-08 18:36 ` [PATCH 3/7] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-08 19:12 ` Jeff King
2021-03-14 16:56 ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 4/7] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-08 19:16 ` Jeff King
2021-03-14 17:56 ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 5/7] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-08 19:29 ` Jeff King
2021-03-08 18:36 ` [PATCH 6/7] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-08 19:30 ` Jeff King
2021-03-08 18:36 ` [PATCH 7/7] parse-options: don't leak alias help messages Andrzej Hunt via GitGitGadget
2021-03-08 19:46 ` Jeff King
2021-03-14 17:03 ` Andrzej Hunt [this message]
2021-03-08 18:55 ` [PATCH 0/7] Fix all leaks in t0001 Jeff King
2021-03-12 23:42 ` Junio C Hamano
2021-03-14 16:54 ` Andrzej Hunt
2021-03-15 16:23 ` Andrzej Hunt
2021-03-08 18:57 ` Junio C Hamano
2021-03-14 16:55 ` Andrzej Hunt
2021-03-14 18:47 ` [PATCH v2 0/9] " Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 1/9] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 2/9] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 3/9] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 4/9] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 5/9] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 6/9] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-14 18:47 ` [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift Andrzej Hunt via GitGitGadget
2021-03-14 20:25 ` Martin Ågren
2021-03-14 22:57 ` Junio C Hamano
2021-03-15 16:20 ` Andrzej Hunt
2021-03-14 18:47 ` [PATCH v2 8/9] parse-options: don't leak alias help messages Andrzej Hunt via GitGitGadget
2021-03-14 19:48 ` Eric Sunshine
2021-03-15 16:20 ` Andrzej Hunt
2021-03-14 20:00 ` Andrzej Hunt
2021-03-14 18:47 ` [PATCH v2 9/9] transport: also free remote_refs in transport_disconnect() Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 0/9] Fix all leaks in t0001 Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 1/9] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 2/9] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 3/9] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 4/9] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 5/9] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 6/9] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 7/9] parse-options: convert bitfield values to use binary shift Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 8/9] parse-options: don't leak alias help messages Andrzej Hunt via GitGitGadget
2021-03-21 16:58 ` [PATCH v3 9/9] transport: also free remote_refs in transport_disconnect() Andrzej Hunt via GitGitGadget
2021-03-21 21:40 ` [PATCH v3 0/9] Fix all leaks in t0001 Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6ddb4ca3-8486-a8cf-24ed-1ae6220602f3@ahunt.org \
--to=andrzej@ahunt.org \
--cc=ajrhunt@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).