{"thread":{"id":"27446","subject":"[PATCH] handle_options(): do not miscount how many arguments were used","startedAt":"2011-05-24T21:15:02Z","lastAt":"2011-05-24T22:50:35Z","messageCount":10,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168598","messageId":"7vsjs37qcp.fsf@alter.siamese.dyndns.org","threadId":"27446","inReplyTo":null,"subject":"[PATCH] handle_options(): do not miscount how many arguments were used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T21:15:02Z","receivedAt":"2011-05-24T21:15:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The handle_options() function advances the base of the argument array and\nreturns the number of arguments it used. The caller in handle_alias()\nwants to reallocate the argv array it passes to this function, and\nattempts to do so by subtracting the returned value to compensate for the\nchange handle_options() makes to the new_argv.\n\nBut handle_options() did not correctly count when \"-c <config=value>\" is\ngiven, causing a wrong pointer to be passed to realloc().\n\nFix it by saving the original argv at the beginning of handle_options(),\nand return the difference between the final value of argv, which will\nrelieve the places that move the array pointer from the additional burden\nof keeping track of \"handled\" counter.\n\nNoticed-by: Kazuki Tsujimoto\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This fixes 8b1fa77 (Allow passing of configuration parameters in the\n   command line, 2010-03-26), and when applied there, the new test passes,\n   but if applied to newer codebase, the test fails for a different\n   reason, for which another fix will be sent out separately.\n\n git.c                  |    6 ++----\n t/t1300-repo-config.sh |   10 ++++++++++\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 1753811..55c2eda 100644\n--- a/git.c\n+++ b/git.c\n@@ -53,7 +53,7 @@ static void commit_pager_choice(void) {\n \n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n-\tint handled = 0;\n+\tconst char **orig_argv = *argv;\n \n \tif (!getenv(\"GIT_ASKPASS\") && getenv(\"SSH_ASKPASS\"))\n \t\tsetenv(\"GIT_ASKPASS\", getenv(\"SSH_ASKPASS\"), 1);\n@@ -106,7 +106,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t\thandled++;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n@@ -146,9 +145,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \n \t\t(*argv)++;\n \t\t(*argc)--;\n-\t\thandled++;\n \t}\n-\treturn handled;\n+\treturn (*argv) - orig_argv;\n }\n \n static int handle_alias(int *argcp, const char ***argv)\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 64f0508..5f6766d 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -832,4 +832,14 @@ test_expect_success 'git -c \"key=value\" support' '\n \ttest_must_fail git -c core.name=value config name\n '\n \n+test_expect_success 'alias to give a configuration value' '\n+\techo \"foo and space \" >foo &&\n+\tgit diff HEAD >foo.patch &&\n+\tgit checkout foo &&\n+\tgit config alias.aw \"-c apply.whitespace=fix apply\" &&\n+\tgit aw foo.patch &&\n+\techo \"foo and space\" >expect &&\n+\ttest_cmp expect foo\n+'\n+\n test_done\n-- \n1.7.5.2.459.g67e41\n"},{"id":"168599","messageId":"7vmxib7q79.fsf@alter.siamese.dyndns.org","threadId":"27446","inReplyTo":"7vsjs37qcp.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Allow built-ins to also use -c var=val via alias","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T21:18:18Z","receivedAt":"2011-05-24T21:18:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The previous commit 2b64fc8 (pass \"git -c foo=bar\" params through\nenvironment, 2010-08-23) tried to make all the one-shot configuration\nsetting made via the \"-c\" option to the \"git\" wrapper go through an\nenvironment variable, so that the value can be propagated to external\ncommands _as well as_ the internal ones, but ended up breaking one of the\ncodepaths that invokes internal commands, because it incorrectly assumed\nthat git_config_from_parmeters() can never be called in a single process\nafter git_config_parse_environment() was called once.\n\nNot so.\n\nWhen the options came as part of an alias to an internal command, e.g.\n\n  [alias]\n    aw = -c apply.whitespace=fix apply\n\nand then run as \"git aw P.diff\", we have already read the configuration to\nfind out about the alias definition (setting loaded_environment to true),\nthen pushed apply.whitespace=fix to the environment, but not to the\nin-core list of configuration variables. The implementation of the\ninternal command, e.g. cmd_apply(), will try to read from git_config() but\nthe setting is lost, as the environment is never read in this codepath.\n\nAdd the in-core queuing of the parameters back to fix this.\n\nNote that the handling of such an alias is still broken for another reason\nin this codepath; a separate patch fixes it.\n\nWhile at it, avoid getting our test confused by GIT_CONFIG_PARAMETERS\nexported to the tester's environment.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * And this is the other fix. Applying this on top of 2b64fc8 does not\n   make \"git aw P.diff\" in the test t1300 in the other patch, but this\n   is prerequisite for the other fix to work in a more modern codebase.\n\n config.c      |    1 +\n t/test-lib.sh |    1 +\n 2 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c2c995f..c803ae8 100644\n--- a/config.c\n+++ b/config.c\n@@ -43,6 +43,7 @@ void git_config_push_parameter(const char *text)\n \t\tstrbuf_addstr(&env, old);\n \t\tstrbuf_addch(&env, ' ');\n \t}\n+\tgit_config_parse_parameter(text);\n \tsq_quote_buf(&env, text);\n \tsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);\n \tstrbuf_release(&env);\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e5523dd..0b1358e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -57,6 +57,7 @@ GIT_AUTHOR_NAME='A U Thor'\n unset GIT_COMMITTER_DATE\n GIT_COMMITTER_EMAIL=committer@example.com\n GIT_COMMITTER_NAME='C O Mitter'\n+unset GIT_CONFIG_PARAMETERS\n unset GIT_DIFF_OPTS\n unset GIT_DIR\n unset GIT_WORK_TREE\n-- \n1.7.5.2.459.g67e41\n"},{"id":"168602","messageId":"20110524214618.GA17727@sigill.intra.peff.net","threadId":"27446","inReplyTo":"7vmxib7q79.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Allow built-ins to also use -c var=val via alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T21:46:18Z","receivedAt":"2011-05-24T21:46:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 02:18:18PM -0700, Junio C Hamano wrote:\n\n> When the options came as part of an alias to an internal command, e.g.\n> \n>   [alias]\n>     aw = -c apply.whitespace=fix apply\n> \n> and then run as \"git aw P.diff\", we have already read the configuration to\n> find out about the alias definition (setting loaded_environment to true),\n> then pushed apply.whitespace=fix to the environment, but not to the\n> in-core list of configuration variables. The implementation of the\n> internal command, e.g. cmd_apply(), will try to read from git_config() but\n> the setting is lost, as the environment is never read in this codepath.\n> \n> Add the in-core queuing of the parameters back to fix this.\n\nHmm. I'm not sure that is right. The fatal assumption in 2b64fc89 is\nthat we do not load the parameters again once they have already been\nloaded. So if your sequence is:\n\n  git_config(...);\n  git_config_push_parameter(...);\n  git_config(...);\n\nthen the first git_config will try git_config_from_parameters, which\nwill call git_config_parse_environment, which will set the static\nloaded_environment variable. And so in the second git_config call, we\nwill not reparse the environment, and your fix is correct.\n\nBut what if the sequence is:\n\n  git_config_push_parameter(...);\n  git_config(...);\n\nWith your fix, the push_parameter will add the variable to the in-memory\nlist, and put it in the environment. But our git_config call will then\nre-parse the environment, adding a duplicate of the variable.\n\nFor most variables, that means a simple overwrite. But there are some\nthat are additive if multiple instances are found, and they may be\nbroken.\n\nI say \"may\" because I am not sure if there are code paths which don't\ncall git_config before git_config_push_parameter. I suspect there are,\nsince otherwise \"-c\" wouldn't work for builtins at all. Even if there\naren't, I'd rather not leave such a fragile thing in place.\n\nI think the right fix is simply to drop the \"don't re-check the\nenvironment after the first time\" logic. It's not expensive to parse\ncompared to parsing config files, which is when we would do it. We can\njust drop the existing list and reparse. You can even get rid of the\nwhole list and drop a bunch of code, I think, like:\n\n---\n config.c |   53 +++++++++++++++--------------------------------------\n 1 files changed, 15 insertions(+), 38 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 80dc715..bc5666f 100644\n--- a/config.c\n+++ b/config.c\n@@ -20,14 +20,6 @@ static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n \n-struct config_item {\n-\tstruct config_item *next;\n-\tchar *name;\n-\tchar *value;\n-};\n-static struct config_item *config_parameters;\n-static struct config_item **config_parameters_tail = &config_parameters;\n-\n static void lowercase(char *p)\n {\n \tfor (; *p; p++)\n@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n-int git_config_parse_parameter(const char *text)\n+int git_config_parse_parameter(const char *text,\n+\t\t\t       const char **name, const char **value)\n {\n-\tstruct config_item *ct;\n \tstruct strbuf tmp = STRBUF_INIT;\n \tstruct strbuf **pair;\n \tstrbuf_addstr(&tmp, text);\n@@ -61,25 +53,24 @@ int git_config_parse_parameter(const char *text)\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n \t}\n-\tct = xcalloc(1, sizeof(struct config_item));\n-\tct->name = strbuf_detach(pair[0], NULL);\n+\t*key = strbuf_detach(pair[0], NULL);\n \tif (pair[1]) {\n \t\tstrbuf_trim(pair[1]);\n-\t\tct->value = strbuf_detach(pair[1], NULL);\n+\t\t*value = strbuf_detach(pair[1], NULL);\n \t}\n \tstrbuf_list_free(pair);\n-\tlowercase(ct->name);\n-\t*config_parameters_tail = ct;\n-\tconfig_parameters_tail = &ct->next;\n+\tlowercase(name);\n \treturn 0;\n }\n \n-int git_config_parse_environment(void) {\n+int git_config_from_parameters(config_fn_t fn, void *data)\n+{\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n \tchar *envw;\n \tconst char **argv = NULL;\n \tint nr = 0, alloc = 0;\n \tint i;\n+\tint ret = -1;\n \n \tif (!env)\n \t\treturn 0;\n@@ -92,17 +83,19 @@ int git_config_parse_environment(void) {\n \t}\n \n \tfor (i = 0; i < nr; i++) {\n-\t\tif (git_config_parse_parameter(argv[i]) < 0) {\n+\t\tif (git_config_parse_parameter(argv[i], &name, &value) < 0) {\n \t\t\terror(\"bogus config parameter: %s\", argv[i]);\n-\t\t\tfree(argv);\n-\t\t\tfree(envw);\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n \t\t}\n+\t\tif (fn(name, value, data) < 0)\n+\t\t\tgoto out;\n \t}\n+\tret = 0;\n \n+out:\n \tfree(argv);\n \tfree(envw);\n-\treturn 0;\n+\treturn ret;\n }\n \n static int get_next_char(void)\n@@ -850,22 +843,6 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-int git_config_from_parameters(config_fn_t fn, void *data)\n-{\n-\tstatic int loaded_environment;\n-\tconst struct config_item *ct;\n-\n-\tif (!loaded_environment) {\n-\t\tif (git_config_parse_environment() < 0)\n-\t\t\treturn -1;\n-\t\tloaded_environment = 1;\n-\t}\n-\tfor (ct = config_parameters; ct; ct = ct->next)\n-\t\tif (fn(ct->name, ct->value, data) < 0)\n-\t\t\treturn -1;\n-\treturn 0;\n-}\n-\n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n \tint ret = 0, found = 0;\n"},{"id":"168603","messageId":"20110524215202.GA22243@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524214618.GA17727@sigill.intra.peff.net","subject":"Re: [PATCH] Allow built-ins to also use -c var=val via alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T21:52:02Z","receivedAt":"2011-05-24T21:52:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:\n\n> I think the right fix is simply to drop the \"don't re-check the\n> environment after the first time\" logic. It's not expensive to parse\n> compared to parsing config files, which is when we would do it. We can\n> just drop the existing list and reparse. You can even get rid of the\n> whole list and drop a bunch of code, I think, like:\n\nAck, wrong patch. That one doesn't even come close to compiling.\n\nTry this (still not well tested, though).\n\n---\n cache.h  |    2 -\n config.c |   68 ++++++++++++++++++++++++-------------------------------------\n 2 files changed, 27 insertions(+), 43 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 28a921d..69e09a1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1037,8 +1037,6 @@ typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n extern void git_config_push_parameter(const char *text);\n-extern int git_config_parse_parameter(const char *text);\n-extern int git_config_parse_environment(void);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\n extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\ndiff --git a/config.c b/config.c\nindex 9d36848..90d5e6d 100644\n--- a/config.c\n+++ b/config.c\n@@ -20,14 +20,6 @@ static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n \n-struct config_item {\n-\tstruct config_item *next;\n-\tchar *name;\n-\tchar *value;\n-};\n-static struct config_item *config_parameters;\n-static struct config_item **config_parameters_tail = &config_parameters;\n-\n static void lowercase(char *p)\n {\n \tfor (; *p; p++)\n@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n-int git_config_parse_parameter(const char *text)\n+int git_config_parse_parameter(const char *text,\n+\t\t\t       char **name, char **value)\n {\n-\tstruct config_item *ct;\n \tstruct strbuf tmp = STRBUF_INIT;\n \tstruct strbuf **pair;\n \tstrbuf_addstr(&tmp, text);\n@@ -61,25 +53,24 @@ int git_config_parse_parameter(const char *text)\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n \t}\n-\tct = xcalloc(1, sizeof(struct config_item));\n-\tct->name = strbuf_detach(pair[0], NULL);\n+\t*name = strbuf_detach(pair[0], NULL);\n \tif (pair[1]) {\n \t\tstrbuf_trim(pair[1]);\n-\t\tct->value = strbuf_detach(pair[1], NULL);\n+\t\t*value = strbuf_detach(pair[1], NULL);\n \t}\n \tstrbuf_list_free(pair);\n-\tlowercase(ct->name);\n-\t*config_parameters_tail = ct;\n-\tconfig_parameters_tail = &ct->next;\n+\tlowercase(*name);\n \treturn 0;\n }\n \n-int git_config_parse_environment(void) {\n+int git_config_from_parameters(config_fn_t fn, void *data)\n+{\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n \tchar *envw;\n \tconst char **argv = NULL;\n \tint nr = 0, alloc = 0;\n \tint i;\n+\tint ret = 0;\n \n \tif (!env)\n \t\treturn 0;\n@@ -92,17 +83,25 @@ int git_config_parse_environment(void) {\n \t}\n \n \tfor (i = 0; i < nr; i++) {\n-\t\tif (git_config_parse_parameter(argv[i]) < 0) {\n+\t\tchar *name, *value;\n+\t\tif (git_config_parse_parameter(argv[i], &name, &value) < 0) {\n \t\t\terror(\"bogus config parameter: %s\", argv[i]);\n-\t\t\tfree(argv);\n-\t\t\tfree(envw);\n-\t\t\treturn -1;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n+\t\tif (fn(name, value, data) < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tfree(name);\n+\t\tfree(value);\n+\t\tret++;\n \t}\n \n+out:\n \tfree(argv);\n \tfree(envw);\n-\treturn 0;\n+\treturn ret;\n }\n \n static int get_next_char(void)\n@@ -837,25 +836,10 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-int git_config_from_parameters(config_fn_t fn, void *data)\n-{\n-\tstatic int loaded_environment;\n-\tconst struct config_item *ct;\n-\n-\tif (!loaded_environment) {\n-\t\tif (git_config_parse_environment() < 0)\n-\t\t\treturn -1;\n-\t\tloaded_environment = 1;\n-\t}\n-\tfor (ct = config_parameters; ct; ct = ct->next)\n-\t\tif (fn(ct->name, ct->value, data) < 0)\n-\t\t\treturn -1;\n-\treturn 0;\n-}\n-\n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n \tint ret = 0, found = 0;\n+\tint n;\n \tconst char *home = NULL;\n \n \t/* Setting $GIT_CONFIG makes git read _only_ the given config file. */\n@@ -882,9 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \t\tfound += 1;\n \t}\n \n-\tret += git_config_from_parameters(fn, data);\n-\tif (config_parameters)\n-\t\tfound += 1;\n+\tn = git_config_from_parameters(fn, data);\n+\tif (n < 0)\n+\t\tret += n;\n+\telse\n+\t\tfound += n;\n \n \treturn ret == 0 ? found : ret;\n }\n"},{"id":"168606","messageId":"20110524215759.GA24298@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524215202.GA22243@sigill.intra.peff.net","subject":"Re: [PATCH] Allow built-ins to also use -c var=val via alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T21:57:59Z","receivedAt":"2011-05-24T21:57:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 05:52:02PM -0400, Jeff King wrote:\n\n> On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:\n> \n> > I think the right fix is simply to drop the \"don't re-check the\n> > environment after the first time\" logic. It's not expensive to parse\n> > compared to parsing config files, which is when we would do it. We can\n> > just drop the existing list and reparse. You can even get rid of the\n> > whole list and drop a bunch of code, I think, like:\n> \n> Ack, wrong patch. That one doesn't even come close to compiling.\n> \n> Try this (still not well tested, though).\n\nUgh, broken. That will teach me to just paste any random junk into my\nMUA. Hopefully you got the gist of what I was trying to say, but let me\ncome up with a more readable and tested series.\n\n-Peff\n"},{"id":"168609","messageId":"20110524224903.GA16265@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524215759.GA24298@sigill.intra.peff.net","subject":"Re: [PATCH] Allow built-ins to also use -c var=val via alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T22:49:03Z","receivedAt":"2011-05-24T22:49:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 05:57:59PM -0400, Jeff King wrote:\n\n> On Tue, May 24, 2011 at 05:52:02PM -0400, Jeff King wrote:\n> \n> > On Tue, May 24, 2011 at 05:46:18PM -0400, Jeff King wrote:\n> > \n> > > I think the right fix is simply to drop the \"don't re-check the\n> > > environment after the first time\" logic. It's not expensive to parse\n> > > compared to parsing config files, which is when we would do it. We can\n> > > just drop the existing list and reparse. You can even get rid of the\n> > > whole list and drop a bunch of code, I think, like:\n> > \n> > Ack, wrong patch. That one doesn't even come close to compiling.\n> > \n> > Try this (still not well tested, though).\n> \n> Ugh, broken. That will teach me to just paste any random junk into my\n> MUA. Hopefully you got the gist of what I was trying to say, but let me\n> come up with a more readable and tested series.\n\nOK, for real this time. This is how I would do the whole fix on top of\nmaster, including your 1/2. I'll let you handle the\napply-to-maint-and-merge as you would have with your original series.\n\nThe first two are refactoring to make 3/4 a little easier to read. The\nthird one is my fix, and the fourth is your original patch (together\nwith my 3/4 it passes the test; btw, I ended up writing a slightly\nsimpler test. Feel free to throw it out if you prefer yours).\n\n  [1/4]: config: make environment parsing routines static\n  [2/4]: git_config: don't peek at global config_parameters\n  [3/4]: config: always parse GIT_CONFIG_PARAMETERS during git_config\n  [4/4]: handle_options(): do not miscount how many arguments were used\n\n-Peff\n"},{"id":"168610","messageId":"20110524224936.GA24527@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524224903.GA16265@sigill.intra.peff.net","subject":"[PATCH 1/4] config: make environment parsing routines static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T22:49:36Z","receivedAt":"2011-05-24T22:49:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Nobody outside of git_config_from_parameters should need\nto use the GIT_CONFIG_PARAMETERS parsing functions, so let's\nmake them private.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h  |    2 --\n config.c |    4 ++--\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 28a921d..69e09a1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1037,8 +1037,6 @@ typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n extern void git_config_push_parameter(const char *text);\n-extern int git_config_parse_parameter(const char *text);\n-extern int git_config_parse_environment(void);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\n extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\ndiff --git a/config.c b/config.c\nindex 9d36848..46aeb9c 100644\n--- a/config.c\n+++ b/config.c\n@@ -47,7 +47,7 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n-int git_config_parse_parameter(const char *text)\n+static int git_config_parse_parameter(const char *text)\n {\n \tstruct config_item *ct;\n \tstruct strbuf tmp = STRBUF_INIT;\n@@ -74,7 +74,7 @@ int git_config_parse_parameter(const char *text)\n \treturn 0;\n }\n \n-int git_config_parse_environment(void) {\n+static int git_config_parse_environment(void) {\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n \tchar *envw;\n \tconst char **argv = NULL;\n-- \n1.7.4.5.7.g2e01\n"},{"id":"168611","messageId":"20110524224945.GB24527@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524224903.GA16265@sigill.intra.peff.net","subject":"[PATCH 2/4] git_config: don't peek at global config_parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T22:49:45Z","receivedAt":"2011-05-24T22:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config_parameters list in config.c is an implementation\ndetail of git_config_from_parameters; instead, that function\nshould tell us whether it found anything.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c |   15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 46aeb9c..fb88839 100644\n--- a/config.c\n+++ b/config.c\n@@ -850,7 +850,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \tfor (ct = config_parameters; ct; ct = ct->next)\n \t\tif (fn(ct->name, ct->value, data) < 0)\n \t\t\treturn -1;\n-\treturn 0;\n+\treturn config_parameters != NULL;\n }\n \n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n@@ -882,9 +882,16 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \t\tfound += 1;\n \t}\n \n-\tret += git_config_from_parameters(fn, data);\n-\tif (config_parameters)\n-\t\tfound += 1;\n+\tswitch (git_config_from_parameters(fn, data)) {\n+\tcase -1: /* error */\n+\t\tret--;\n+\t\tbreak;\n+\tcase 0: /* found nothing */\n+\t\tbreak;\n+\tdefault: /* found at least one item */\n+\t\tfound++;\n+\t\tbreak;\n+\t}\n \n \treturn ret == 0 ? found : ret;\n }\n-- \n1.7.4.5.7.g2e01\n"},{"id":"168612","messageId":"20110524224955.GC24527@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524224903.GA16265@sigill.intra.peff.net","subject":"[PATCH 3/4] config: always parse GIT_CONFIG_PARAMETERS during git_config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T22:49:55Z","receivedAt":"2011-05-24T22:49:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously we parsed GIT_CONFIG_PARAMETERS lazily into a\nlinked list, and then checked that list during future\ninvocations of git_config. However, that ignores the fact\nthat the environment variable could change during our run\n(e.g., because we parse more \"-c\" as part of an alias).\n\nInstead, let's just re-parse the environment variable each\ntime. It's generally not very big, and it's no more work\nthan parsing the config files, anyway.\n\nAs a bonus, we can ditch all of the linked list storage code\nentirely, making the code much simpler.\n\nThe test unfortunately still does not pass because of an\nunrelated bug in handle_options.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c               |   50 ++++++++++-------------------------------------\n t/t1300-repo-config.sh |    7 ++++++\n 2 files changed, 18 insertions(+), 39 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex fb88839..e0b3b80 100644\n--- a/config.c\n+++ b/config.c\n@@ -20,14 +20,6 @@ static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n \n-struct config_item {\n-\tstruct config_item *next;\n-\tchar *name;\n-\tchar *value;\n-};\n-static struct config_item *config_parameters;\n-static struct config_item **config_parameters_tail = &config_parameters;\n-\n static void lowercase(char *p)\n {\n \tfor (; *p; p++)\n@@ -47,9 +39,9 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n-static int git_config_parse_parameter(const char *text)\n+static int git_config_parse_parameter(const char *text,\n+\t\t\t\t      config_fn_t fn, void *data)\n {\n-\tstruct config_item *ct;\n \tstruct strbuf tmp = STRBUF_INIT;\n \tstruct strbuf **pair;\n \tstrbuf_addstr(&tmp, text);\n@@ -59,22 +51,19 @@ static int git_config_parse_parameter(const char *text)\n \tstrbuf_trim(pair[0]);\n \tif (!pair[0]->len) {\n \t\tstrbuf_list_free(pair);\n-\t\treturn -1;\n+\t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tct = xcalloc(1, sizeof(struct config_item));\n-\tct->name = strbuf_detach(pair[0], NULL);\n-\tif (pair[1]) {\n-\t\tstrbuf_trim(pair[1]);\n-\t\tct->value = strbuf_detach(pair[1], NULL);\n+\tlowercase(pair[0]->buf);\n+\tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n+\t\tstrbuf_list_free(pair);\n+\t\treturn -1;\n \t}\n \tstrbuf_list_free(pair);\n-\tlowercase(ct->name);\n-\t*config_parameters_tail = ct;\n-\tconfig_parameters_tail = &ct->next;\n \treturn 0;\n }\n \n-static int git_config_parse_environment(void) {\n+int git_config_from_parameters(config_fn_t fn, void *data)\n+{\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n \tchar *envw;\n \tconst char **argv = NULL;\n@@ -92,8 +81,7 @@ static int git_config_parse_environment(void) {\n \t}\n \n \tfor (i = 0; i < nr; i++) {\n-\t\tif (git_config_parse_parameter(argv[i]) < 0) {\n-\t\t\terror(\"bogus config parameter: %s\", argv[i]);\n+\t\tif (git_config_parse_parameter(argv[i], fn, data) < 0) {\n \t\t\tfree(argv);\n \t\t\tfree(envw);\n \t\t\treturn -1;\n@@ -102,7 +90,7 @@ static int git_config_parse_environment(void) {\n \n \tfree(argv);\n \tfree(envw);\n-\treturn 0;\n+\treturn nr > 0;\n }\n \n static int get_next_char(void)\n@@ -837,22 +825,6 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-int git_config_from_parameters(config_fn_t fn, void *data)\n-{\n-\tstatic int loaded_environment;\n-\tconst struct config_item *ct;\n-\n-\tif (!loaded_environment) {\n-\t\tif (git_config_parse_environment() < 0)\n-\t\t\treturn -1;\n-\t\tloaded_environment = 1;\n-\t}\n-\tfor (ct = config_parameters; ct; ct = ct->next)\n-\t\tif (fn(ct->name, ct->value, data) < 0)\n-\t\t\treturn -1;\n-\treturn config_parameters != NULL;\n-}\n-\n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n \tint ret = 0, found = 0;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 53fb822..fe7a153 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -897,4 +897,11 @@ test_expect_success 'key sanity-checking' '\n \tgit config foo.\"ba =z\".bar false\n '\n \n+test_expect_failure 'git -c works with aliases of builtins' '\n+\tgit config alias.checkconfig \"-c foo.check=bar config foo.check\" &&\n+\techo bar >expect &&\n+\tgit checkconfig >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.4.5.7.g2e01\n"},{"id":"168613","messageId":"20110524225035.GD24527@sigill.intra.peff.net","threadId":"27446","inReplyTo":"20110524224903.GA16265@sigill.intra.peff.net","subject":"[PATCH 4/4] handle_options(): do not miscount how many arguments were used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T22:50:35Z","receivedAt":"2011-05-24T22:50:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nThe handle_options() function advances the base of the argument array and\nreturns the number of arguments it used. The caller in handle_alias()\nwants to reallocate the argv array it passes to this function, and\nattempts to do so by subtracting the returned value to compensate for the\nchange handle_options() makes to the new_argv.\n\nBut handle_options() did not correctly count when \"-c <config=value>\" is\ngiven, causing a wrong pointer to be passed to realloc().\n\nFix it by saving the original argv at the beginning of handle_options(),\nand return the difference between the final value of argv, which will\nrelieve the places that move the array pointer from the additional burden\nof keeping track of \"handled\" counter.\n\nNoticed-by: Kazuki Tsujimoto\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git.c                  |    6 ++----\n t/t1300-repo-config.sh |    2 +-\n 2 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex a5ef3c6..89721d4 100644\n--- a/git.c\n+++ b/git.c\n@@ -66,7 +66,7 @@ static void commit_pager_choice(void) {\n \n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n-\tint handled = 0;\n+\tconst char **orig_argv = *argv;\n \n \twhile (*argc > 0) {\n \t\tconst char *cmd = (*argv)[0];\n@@ -122,7 +122,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t\thandled++;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n@@ -162,9 +161,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \n \t\t(*argv)++;\n \t\t(*argc)--;\n-\t\thandled++;\n \t}\n-\treturn handled;\n+\treturn (*argv) - orig_argv;\n }\n \n static int handle_alias(int *argcp, const char ***argv)\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex fe7a153..3db5626 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -897,7 +897,7 @@ test_expect_success 'key sanity-checking' '\n \tgit config foo.\"ba =z\".bar false\n '\n \n-test_expect_failure 'git -c works with aliases of builtins' '\n+test_expect_success 'git -c works with aliases of builtins' '\n \tgit config alias.checkconfig \"-c foo.check=bar config foo.check\" &&\n \techo bar >expect &&\n \tgit checkconfig >actual &&\n-- \n1.7.4.5.7.g2e01\n"}]}