Re: [BUG] realloc failed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 22, 2011, 19:42 UTC
- Message-ID
- <7v4o4my11c.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <7v8vtyy1ft.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The immediate cause of the problem is that handle_options() returns a > wrong count when it sees a "-c <config-specification>". If you look at its > implementation, you see "handled++" paired with "(*argv)++" everywhere but > the place it parsed "-c".
I said "immediate" above, because I think the "real" reason why we suffered is because handle_options() helper function was created by refactoring existing code with a poor taste in designing an API.
It is understandable that it is more convenent for one callsite, which is git.c::main(), to make the helper automatically move the argc/argv variables of the caller, and it certainly would have made the initial patch look smaller when the helper was introduced, but as an API for a more general callers, it is easier to follow the resulting code of the caller if it didn't.
Refactored in a more reasonable way, the result may look like this, on top of the previous patch.
*CAUTION* I was not very careful checking sites that increment these variables and rely on the result when I was doing this, so there may be places that need to be adjusted by adding or subtracting option_count)
git.c | 51 +++++++++++++++++++++++++++------------------------ 1 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/git.c b/git.c index df4306d..3596f5b 100644 --- a/git.c +++ b/git.c @@ -64,12 +64,12 @@ static void commit_pager_choice(void) { } } -static int handle_options(const char ***argv, int *argc, int *envchanged) +static int handle_options(int argc, const char **argv, int *envchanged) { - const char **orig_argv = *argv; + const char **orig_argv = argv; - while (*argc > 0) { - const char *cmd = (*argv)[0]; + while (argc > 0) { + const char *cmd = argv[0]; if (cmd[0] != '-') break; @@ -107,29 +107,29 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "--git-dir")) { - if (*argc < 2) { + if (argc < 2) { fprintf(stderr, "No directory given for --git-dir.\n" ); usage(git_usage_string); } - setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1); + setenv(GIT_DIR_ENVIRONMENT, argv[1], 1); if (envchanged) *envchanged = 1; - (*argv)++; - (*argc)--; + argv++; + argc--; } else if (!prefixcmp(cmd, "--git-dir=")) { setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1); if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "--work-tree")) { - if (*argc < 2) { + if (argc < 2) { fprintf(stderr, "No directory given for --work-tree.\n" ); usage(git_usage_string); } - setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1); + setenv(GIT_WORK_TREE_ENVIRONMENT, argv[1], 1); if (envchanged) *envchanged = 1; - (*argv)++; - (*argc)--; + argv++; + argc--; } else if (!prefixcmp(cmd, "--work-tree=")) { setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1); if (envchanged) @@ -141,22 +141,22 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "-c")) { - if (*argc < 2) { + if (argc < 2) { fprintf(stderr, "-c expects a configuration string\n" ); usage(git_usage_string); } - git_config_push_parameter((*argv)[1]); - (*argv)++; - (*argc)--; + git_config_push_parameter(argv[1]); + argv++; + argc--; } else { fprintf(stderr, "Unknown option: %s\n", cmd); usage(git_usage_string); } - (*argv)++; - (*argc)--; + argv++; + argc--; } - return (*argv) - orig_argv; + return argv - orig_argv; } static int handle_alias(int *argcp, const char ***argv) @@ -198,14 +198,14 @@ static int handle_alias(int *argcp, const char ***argv) if (count < 0) die("Bad alias.%s string: %s", alias_command, split_cmdline_strerror(count)); - option_count = handle_options(&new_argv, &count, &envchanged); + option_count = handle_options(count, new_argv, &envchanged); + count -= option_count; if (envchanged) die("alias '%s' changes environment variables\n" "You can use '!git' in the alias to do this.", alias_command); - memmove(new_argv - option_count, new_argv, - count * sizeof(char *)); - new_argv -= option_count; + memmove(new_argv, new_argv + option_count, + count * sizeof(char *)); if (count < 1) die("empty alias for %s", alias_command); @@ -508,6 +508,7 @@ static int run_argv(int *argcp, const char ***argv) int main(int argc, const char **argv) { const char *cmd; + int option_count; startup_info = &git_startup_info; @@ -535,7 +536,9 @@ int main(int argc, const char **argv) /* Look for flags.. */ argv++; argc--; - handle_options(&argv, &argc, NULL); + option_count = handle_options(argc, argv, NULL); + argv += option_count; + argc -= option_count; if (argc > 0) { if (!prefixcmp(argv[0], "--")) argv[0] += 2;