[PATCH] handle_options(): do not miscount how many arguments were used

[PATCH] handle_options(): do not miscount how many arguments were used

[Junio C Hamano]

The handle_options() function advances the base of the argument array and
returns the number of arguments it used. The caller in handle_alias()
wants to reallocate the argv array it passes to this function, and
attempts to do so by subtracting the returned value to compensate for the
change handle_options() makes to the new_argv.

But handle_options() did not correctly count when "-c <config=value>" is
given, causing a wrong pointer to be passed to realloc().

Fix it by saving the original argv at the beginning of handle_options(),
and return the difference between the final value of argv, which will
relieve the places that move the array pointer from the additional burden
of keeping track of "handled" counter.

Noticed-by: Kazuki Tsujimoto
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This fixes 8b1fa77 (Allow passing of configuration parameters in the
   command line, 2010-03-26), and when applied there, the new test passes,
   but if applied to newer codebase, the test fails for a different
   reason, for which another fix will be sent out separately.

 git.c                  |    6 ++----
 t/t1300-repo-config.sh |   10 ++++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 1753811..55c2eda 100644
--- a/git.c
+++ b/git.c
@@ -53,7 +53,7 @@ static void commit_pager_choice(void) {
 
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
-	int handled = 0;
+	const char **orig_argv = *argv;
 
 	if (!getenv("GIT_ASKPASS") && getenv("SSH_ASKPASS"))
 		setenv("GIT_ASKPASS", getenv("SSH_ASKPASS"), 1);
@@ -106,7 +106,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-			handled++;
 		} else if (!prefixcmp(cmd, "--git-dir=")) {
 			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 			if (envchanged)
@@ -146,9 +145,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 		(*argv)++;
 		(*argc)--;
-		handled++;
 	}
-	return handled;
+	return (*argv) - orig_argv;
 }
 
 static int handle_alias(int *argcp, const char ***argv)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 64f0508..5f6766d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -832,4 +832,14 @@ test_expect_success 'git -c "key=value" support' '
 	test_must_fail git -c core.name=value config name
 '
 
+test_expect_success 'alias to give a configuration value' '
+	echo "foo and space " >foo &&
+	git diff HEAD >foo.patch &&
+	git checkout foo &&
+	git config alias.aw "-c apply.whitespace=fix apply" &&
+	git aw foo.patch &&
+	echo "foo and space" >expect &&
+	test_cmp expect foo
+'
+
 test_done
-- 
1.7.5.2.459.g67e41

[PATCH] Allow built-ins to also use -c var=val via alias

[Junio C Hamano]

The previous commit 2b64fc8 (pass "git -c foo=bar" params through
environment, 2010-08-23) tried to make all the one-shot configuration
setting made via the "-c" option to the "git" wrapper go through an
environment variable, so that the value can be propagated to external
commands _as well as_ the internal ones, but ended up breaking one of the
codepaths that invokes internal commands, because it incorrectly assumed
that git_config_from_parmeters() can never be called in a single process
after git_config_parse_environment() was called once.

Not so.

When the options came as part of an alias to an internal command, e.g.

  [alias]
    aw = -c apply.whitespace=fix apply

and then run as "git aw P.diff", we have already read the configuration to
find out about the alias definition (setting loaded_environment to true),
then pushed apply.whitespace=fix to the environment, but not to the
in-core list of configuration variables. The implementation of the
internal command, e.g. cmd_apply(), will try to read from git_config() but
the setting is lost, as the environment is never read in this codepath.

Add the in-core queuing of the parameters back to fix this.

Note that the handling of such an alias is still broken for another reason
in this codepath; a separate patch fixes it.

While at it, avoid getting our test confused by GIT_CONFIG_PARAMETERS
exported to the tester's environment.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is the other fix. Applying this on top of 2b64fc8 does not
   make "git aw P.diff" in the test t1300 in the other patch, but this
   is prerequisite for the other fix to work in a more modern codebase.

 config.c      |    1 +
 t/test-lib.sh |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/config.c b/config.c
index c2c995f..c803ae8 100644
--- a/config.c
+++ b/config.c
@@ -43,6 +43,7 @@ void git_config_push_parameter(const char *text)
 		strbuf_addstr(&env, old);
 		strbuf_addch(&env, ' ');
 	}
+	git_config_parse_parameter(text);
 	sq_quote_buf(&env, text);
 	setenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);
 	strbuf_release(&env);
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e5523dd..0b1358e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -57,6 +57,7 @@ GIT_AUTHOR_NAME='A U Thor'
 unset GIT_COMMITTER_DATE
 GIT_COMMITTER_EMAIL=committer@example.com
 GIT_COMMITTER_NAME='C O Mitter'
+unset GIT_CONFIG_PARAMETERS
 unset GIT_DIFF_OPTS
 unset GIT_DIR
 unset GIT_WORK_TREE
-- 
1.7.5.2.459.g67e41

Re: [PATCH] Allow built-ins to also use -c var=val via alias

[Jeff King]

On Tue, May 24, 2011 at 02:18:18PM -0700, Junio C Hamano wrote:

> When the options came as part of an alias to an internal command, e.g.
> 
>   [alias]
>     aw = -c apply.whitespace=fix apply
> 
> and then run as "git aw P.diff", we have already read the configuration to
> find out about the alias definition (setting loaded_environment to true),
> then pushed apply.whitespace=fix to the environment, but not to the
> in-core list of configuration variables. The implementation of the
> internal command, e.g. cmd_apply(), will try to read from git_config() but
> the setting is lost, as the environment is never read in this codepath.
> 
> Add the in-core queuing of the parameters back to fix this.

Hmm. I'm not sure that is right. The fatal assumption in 2b64fc89 is
that we do not load the parameters again once they have already been
loaded. So if your sequence is:

  git_config(...);
  git_config_push_parameter(...);
  git_config(...);

then the first git_config will try git_config_from_parameters, which
will call git_config_parse_environment, which will set the static
loaded_environment variable. And so in the second git_config call, we
will not reparse the environment, and your fix is correct.

But what if the sequence is:

  git_config_push_parameter(...);
  git_config(...);

With your fix, the push_parameter will add the variable to the in-memory
list, and put it in the environment. But our git_config call will then
re-parse the environment, adding a duplicate of the variable.

For most variables, that means a simple overwrite. But there are some
that are additive if multiple instances are found, and they may be
broken.

I say "may" because I am not sure if there are code paths which don't
call git_config before git_config_push_parameter. I suspect there are,
since otherwise "-c" wouldn't work for builtins at all. Even if there
aren't, I'd rather not leave such a fragile thing in place.

I think the right fix is simply to drop the "don't re-check the
environment after the first time" logic. It's not expensive to parse
compared to parsing config files, which is when we would do it. We can
just drop the existing list and reparse. You can even get rid of the
whole list and drop a bunch of code, I think, like:

---
 config.c |   53 +++++++++++++++--------------------------------------
 1 files changed, 15 insertions(+), 38 deletions(-)

diff --git a/config.c b/config.c
index 80dc715..bc5666f 100644
--- a/config.c
+++ b/config.c
@@ -20,14 +20,6 @@ static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
 
-struct config_item {
-	struct config_item *next;
-	char *name;
-	char *value;
-};
-static struct config_item *config_parameters;
-static struct config_item **config_parameters_tail = &config_parameters;
-
 static void lowercase(char *p)
 {
 	for (; *p; p++)
@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-int git_config_parse_parameter(const char *text)
+int git_config_parse_parameter(const char *text,
+			       const char **name, const char **value)
 {
-	struct config_item *ct;
 	struct strbuf tmp = STRBUF_INIT;
 	struct strbuf **pair;
 	strbuf_addstr(&tmp, text);
@@ -61,25 +53,24 @@ int git_config_parse_parameter(const char *text)
 		strbuf_list_free(pair);
 		return -1;
 	}
-	ct = xcalloc(1, sizeof(struct config_item));
-	ct->name = strbuf_detach(pair[0], NULL);
+	*key = strbuf_detach(pair[0], NULL);
 	if (pair[1]) {
 		strbuf_trim(pair[1]);
-		ct->value = strbuf_detach(pair[1], NULL);
+		*value = strbuf_detach(pair[1], NULL);
 	}
 	strbuf_list_free(pair);
-	lowercase(ct->name);
-	*config_parameters_tail = ct;
-	config_parameters_tail = &ct->next;
+	lowercase(name);
 	return 0;
 }
 
-int git_config_parse_environment(void) {
+int git_config_from_parameters(config_fn_t fn, void *data)
+{
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
 	const char **argv = NULL;
 	int nr = 0, alloc = 0;
 	int i;
+	int ret = -1;
 
 	if (!env)
 		return 0;
@@ -92,17 +83,19 @@ int git_config_parse_environment(void) {
 	}
 
 	for (i = 0; i < nr; i++) {
-		if (git_config_parse_parameter(argv[i]) < 0) {
+		if (git_config_parse_parameter(argv[i], &name, &value) < 0) {
 			error("bogus config parameter: %s", argv[i]);
-			free(argv);
-			free(envw);
-			return -1;
+			goto out;
 		}
+		if (fn(name, value, data) < 0)
+			goto out;
 	}
+	ret = 0;
 
+out:
 	free(argv);
 	free(envw);
-	return 0;
+	return ret;
 }
 
 static int get_next_char(void)
@@ -850,22 +843,6 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-int git_config_from_parameters(config_fn_t fn, void *data)
-{
-	static int loaded_environment;
-	const struct config_item *ct;
-
-	if (!loaded_environment) {
-		if (git_config_parse_environment() < 0)
-			return -1;
-		loaded_environment = 1;
-	}
-	for (ct = config_parameters; ct; ct = ct->next)
-		if (fn(ct->name, ct->value, data) < 0)
-			return -1;
-	return 0;
-}
-
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;

Re: [PATCH] Allow built-ins to also use -c var=val via alias

[Jeff King]

On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:

> I think the right fix is simply to drop the "don't re-check the
> environment after the first time" logic. It's not expensive to parse
> compared to parsing config files, which is when we would do it. We can
> just drop the existing list and reparse. You can even get rid of the
> whole list and drop a bunch of code, I think, like:

Ack, wrong patch. That one doesn't even come close to compiling.

Try this (still not well tested, though).

---
 cache.h  |    2 -
 config.c |   68 ++++++++++++++++++++++++-------------------------------------
 2 files changed, 27 insertions(+), 43 deletions(-)

diff --git a/cache.h b/cache.h
index 28a921d..69e09a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1037,8 +1037,6 @@ typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern void git_config_push_parameter(const char *text);
-extern int git_config_parse_parameter(const char *text);
-extern int git_config_parse_environment(void);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
diff --git a/config.c b/config.c
index 9d36848..90d5e6d 100644
--- a/config.c
+++ b/config.c
@@ -20,14 +20,6 @@ static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
 
-struct config_item {
-	struct config_item *next;
-	char *name;
-	char *value;
-};
-static struct config_item *config_parameters;
-static struct config_item **config_parameters_tail = &config_parameters;
-
 static void lowercase(char *p)
 {
 	for (; *p; p++)
@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-int git_config_parse_parameter(const char *text)
+int git_config_parse_parameter(const char *text,
+			       char **name, char **value)
 {
-	struct config_item *ct;
 	struct strbuf tmp = STRBUF_INIT;
 	struct strbuf **pair;
 	strbuf_addstr(&tmp, text);
@@ -61,25 +53,24 @@ int git_config_parse_parameter(const char *text)
 		strbuf_list_free(pair);
 		return -1;
 	}
-	ct = xcalloc(1, sizeof(struct config_item));
-	ct->name = strbuf_detach(pair[0], NULL);
+	*name = strbuf_detach(pair[0], NULL);
 	if (pair[1]) {
 		strbuf_trim(pair[1]);
-		ct->value = strbuf_detach(pair[1], NULL);
+		*value = strbuf_detach(pair[1], NULL);
 	}
 	strbuf_list_free(pair);
-	lowercase(ct->name);
-	*config_parameters_tail = ct;
-	config_parameters_tail = &ct->next;
+	lowercase(*name);
 	return 0;
 }
 
-int git_config_parse_environment(void) {
+int git_config_from_parameters(config_fn_t fn, void *data)
+{
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
 	const char **argv = NULL;
 	int nr = 0, alloc = 0;
 	int i;
+	int ret = 0;
 
 	if (!env)
 		return 0;
@@ -92,17 +83,25 @@ int git_config_parse_environment(void) {
 	}
 
 	for (i = 0; i < nr; i++) {
-		if (git_config_parse_parameter(argv[i]) < 0) {
+		char *name, *value;
+		if (git_config_parse_parameter(argv[i], &name, &value) < 0) {
 			error("bogus config parameter: %s", argv[i]);
-			free(argv);
-			free(envw);
-			return -1;
+			ret = -1;
+			goto out;
 		}
+		if (fn(name, value, data) < 0) {
+			ret = -1;
+			goto out;
+		}
+		free(name);
+		free(value);
+		ret++;
 	}
 
+out:
 	free(argv);
 	free(envw);
-	return 0;
+	return ret;
 }
 
 static int get_next_char(void)
@@ -837,25 +836,10 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-int git_config_from_parameters(config_fn_t fn, void *data)
-{
-	static int loaded_environment;
-	const struct config_item *ct;
-
-	if (!loaded_environment) {
-		if (git_config_parse_environment() < 0)
-			return -1;
-		loaded_environment = 1;
-	}
-	for (ct = config_parameters; ct; ct = ct->next)
-		if (fn(ct->name, ct->value, data) < 0)
-			return -1;
-	return 0;
-}
-
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
+	int n;
 	const char *home = NULL;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
@@ -882,9 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	ret += git_config_from_parameters(fn, data);
-	if (config_parameters)
-		found += 1;
+	n = git_config_from_parameters(fn, data);
+	if (n < 0)
+		ret += n;
+	else
+		found += n;
 
 	return ret == 0 ? found : ret;
 }

Re: [PATCH] Allow built-ins to also use -c var=val via alias

[Jeff King]

On Tue, May 24, 2011 at 05:52:02PM -0400, Jeff King wrote:

> On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:
> 
> > I think the right fix is simply to drop the "don't re-check the
> > environment after the first time" logic. It's not expensive to parse
> > compared to parsing config files, which is when we would do it. We can
> > just drop the existing list and reparse. You can even get rid of the
> > whole list and drop a bunch of code, I think, like:
> 
> Ack, wrong patch. That one doesn't even come close to compiling.
> 
> Try this (still not well tested, though).

Ugh, broken. That will teach me to just paste any random junk into my
MUA. Hopefully you got the gist of what I was trying to say, but let me
come up with a more readable and tested series.

-Peff

Re: [PATCH] Allow built-ins to also use -c var=val via alias

[Jeff King]

On Tue, May 24, 2011 at 05:57:59PM -0400, Jeff King wrote:

> On Tue, May 24, 2011 at 05:52:02PM -0400, Jeff King wrote:
> 
> > On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:
> > 
> > > I think the right fix is simply to drop the "don't re-check the
> > > environment after the first time" logic. It's not expensive to parse
> > > compared to parsing config files, which is when we would do it. We can
> > > just drop the existing list and reparse. You can even get rid of the
> > > whole list and drop a bunch of code, I think, like:
> > 
> > Ack, wrong patch. That one doesn't even come close to compiling.
> > 
> > Try this (still not well tested, though).
> 
> Ugh, broken. That will teach me to just paste any random junk into my
> MUA. Hopefully you got the gist of what I was trying to say, but let me
> come up with a more readable and tested series.

OK, for real this time. This is how I would do the whole fix on top of
master, including your 1/2. I'll let you handle the
apply-to-maint-and-merge as you would have with your original series.

The first two are refactoring to make 3/4 a little easier to read. The
third one is my fix, and the fourth is your original patch (together
with my 3/4 it passes the test; btw, I ended up writing a slightly
simpler test. Feel free to throw it out if you prefer yours).

  [1/4]: config: make environment parsing routines static
  [2/4]: git_config: don't peek at global config_parameters
  [3/4]: config: always parse GIT_CONFIG_PARAMETERS during git_config
  [4/4]: handle_options(): do not miscount how many arguments were used

-Peff

[PATCH 1/4] config: make environment parsing routines static

[Jeff King]

Nobody outside of git_config_from_parameters should need
to use the GIT_CONFIG_PARAMETERS parsing functions, so let's
make them private.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  |    2 --
 config.c |    4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 28a921d..69e09a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1037,8 +1037,6 @@ typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern void git_config_push_parameter(const char *text);
-extern int git_config_parse_parameter(const char *text);
-extern int git_config_parse_environment(void);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
diff --git a/config.c b/config.c
index 9d36848..46aeb9c 100644
--- a/config.c
+++ b/config.c
@@ -47,7 +47,7 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-int git_config_parse_parameter(const char *text)
+static int git_config_parse_parameter(const char *text)
 {
 	struct config_item *ct;
 	struct strbuf tmp = STRBUF_INIT;
@@ -74,7 +74,7 @@ int git_config_parse_parameter(const char *text)
 	return 0;
 }
 
-int git_config_parse_environment(void) {
+static int git_config_parse_environment(void) {
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
 	const char **argv = NULL;
-- 
1.7.4.5.7.g2e01

[PATCH 2/4] git_config: don't peek at global config_parameters

[Jeff King]

The config_parameters list in config.c is an implementation
detail of git_config_from_parameters; instead, that function
should tell us whether it found anything.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index 46aeb9c..fb88839 100644
--- a/config.c
+++ b/config.c
@@ -850,7 +850,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 	for (ct = config_parameters; ct; ct = ct->next)
 		if (fn(ct->name, ct->value, data) < 0)
 			return -1;
-	return 0;
+	return config_parameters != NULL;
 }
 
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
@@ -882,9 +882,16 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	ret += git_config_from_parameters(fn, data);
-	if (config_parameters)
-		found += 1;
+	switch (git_config_from_parameters(fn, data)) {
+	case -1: /* error */
+		ret--;
+		break;
+	case 0: /* found nothing */
+		break;
+	default: /* found at least one item */
+		found++;
+		break;
+	}
 
 	return ret == 0 ? found : ret;
 }
-- 
1.7.4.5.7.g2e01

[PATCH 3/4] config: always parse GIT_CONFIG_PARAMETERS during git_config

[Jeff King]

Previously we parsed GIT_CONFIG_PARAMETERS lazily into a
linked list, and then checked that list during future
invocations of git_config. However, that ignores the fact
that the environment variable could change during our run
(e.g., because we parse more "-c" as part of an alias).

Instead, let's just re-parse the environment variable each
time. It's generally not very big, and it's no more work
than parsing the config files, anyway.

As a bonus, we can ditch all of the linked list storage code
entirely, making the code much simpler.

The test unfortunately still does not pass because of an
unrelated bug in handle_options.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c               |   50 ++++++++++-------------------------------------
 t/t1300-repo-config.sh |    7 ++++++
 2 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/config.c b/config.c
index fb88839..e0b3b80 100644
--- a/config.c
+++ b/config.c
@@ -20,14 +20,6 @@ static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
 
-struct config_item {
-	struct config_item *next;
-	char *name;
-	char *value;
-};
-static struct config_item *config_parameters;
-static struct config_item **config_parameters_tail = &config_parameters;
-
 static void lowercase(char *p)
 {
 	for (; *p; p++)
@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-static int git_config_parse_parameter(const char *text)
+static int git_config_parse_parameter(const char *text,
+				      config_fn_t fn, void *data)
 {
-	struct config_item *ct;
 	struct strbuf tmp = STRBUF_INIT;
 	struct strbuf **pair;
 	strbuf_addstr(&tmp, text);
@@ -59,22 +51,19 @@ static int git_config_parse_parameter(const char *text)
 	strbuf_trim(pair[0]);
 	if (!pair[0]->len) {
 		strbuf_list_free(pair);
-		return -1;
+		return error("bogus config parameter: %s", text);
 	}
-	ct = xcalloc(1, sizeof(struct config_item));
-	ct->name = strbuf_detach(pair[0], NULL);
-	if (pair[1]) {
-		strbuf_trim(pair[1]);
-		ct->value = strbuf_detach(pair[1], NULL);
+	lowercase(pair[0]->buf);
+	if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
+		strbuf_list_free(pair);
+		return -1;
 	}
 	strbuf_list_free(pair);
-	lowercase(ct->name);
-	*config_parameters_tail = ct;
-	config_parameters_tail = &ct->next;
 	return 0;
 }
 
-static int git_config_parse_environment(void) {
+int git_config_from_parameters(config_fn_t fn, void *data)
+{
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
 	const char **argv = NULL;
@@ -92,8 +81,7 @@ static int git_config_parse_environment(void) {
 	}
 
 	for (i = 0; i < nr; i++) {
-		if (git_config_parse_parameter(argv[i]) < 0) {
-			error("bogus config parameter: %s", argv[i]);
+		if (git_config_parse_parameter(argv[i], fn, data) < 0) {
 			free(argv);
 			free(envw);
 			return -1;
@@ -102,7 +90,7 @@ static int git_config_parse_environment(void) {
 
 	free(argv);
 	free(envw);
-	return 0;
+	return nr > 0;
 }
 
 static int get_next_char(void)
@@ -837,22 +825,6 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-int git_config_from_parameters(config_fn_t fn, void *data)
-{
-	static int loaded_environment;
-	const struct config_item *ct;
-
-	if (!loaded_environment) {
-		if (git_config_parse_environment() < 0)
-			return -1;
-		loaded_environment = 1;
-	}
-	for (ct = config_parameters; ct; ct = ct->next)
-		if (fn(ct->name, ct->value, data) < 0)
-			return -1;
-	return config_parameters != NULL;
-}
-
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 53fb822..fe7a153 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -897,4 +897,11 @@ test_expect_success 'key sanity-checking' '
 	git config foo."ba =z".bar false
 '
 
+test_expect_failure 'git -c works with aliases of builtins' '
+	git config alias.checkconfig "-c foo.check=bar config foo.check" &&
+	echo bar >expect &&
+	git checkconfig >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.4.5.7.g2e01

[PATCH 4/4] handle_options(): do not miscount how many arguments were used

[Jeff King]

From: Junio C Hamano <gitster@pobox.com>

The handle_options() function advances the base of the argument array and
returns the number of arguments it used. The caller in handle_alias()
wants to reallocate the argv array it passes to this function, and
attempts to do so by subtracting the returned value to compensate for the
change handle_options() makes to the new_argv.

But handle_options() did not correctly count when "-c <config=value>" is
given, causing a wrong pointer to be passed to realloc().

Fix it by saving the original argv at the beginning of handle_options(),
and return the difference between the final value of argv, which will
relieve the places that move the array pointer from the additional burden
of keeping track of "handled" counter.

Noticed-by: Kazuki Tsujimoto
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 git.c                  |    6 ++----
 t/t1300-repo-config.sh |    2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/git.c b/git.c
index a5ef3c6..89721d4 100644
--- a/git.c
+++ b/git.c
@@ -66,7 +66,7 @@ static void commit_pager_choice(void) {
 
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
-	int handled = 0;
+	const char **orig_argv = *argv;
 
 	while (*argc > 0) {
 		const char *cmd = (*argv)[0];
@@ -122,7 +122,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-			handled++;
 		} else if (!prefixcmp(cmd, "--git-dir=")) {
 			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 			if (envchanged)
@@ -162,9 +161,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 		(*argv)++;
 		(*argc)--;
-		handled++;
 	}
-	return handled;
+	return (*argv) - orig_argv;
 }
 
 static int handle_alias(int *argcp, const char ***argv)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index fe7a153..3db5626 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -897,7 +897,7 @@ test_expect_success 'key sanity-checking' '
 	git config foo."ba =z".bar false
 '
 
-test_expect_failure 'git -c works with aliases of builtins' '
+test_expect_success 'git -c works with aliases of builtins' '
 	git config alias.checkconfig "-c foo.check=bar config foo.check" &&
 	echo bar >expect &&
 	git checkconfig >actual &&
-- 
1.7.4.5.7.g2e01