Re: [BUG] realloc failed

[Junio C Hamano <gitster@pobox.com>]

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;