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!
