* "git -c web.browser=w3m help -w help" still kicks firefox @ 2010-08-23 18:05 Junio C Hamano 2010-08-23 18:38 ` Jeff King 2010-08-23 19:02 ` "git -c web.browser=w3m help -w help" still kicks firefox Alex Riesen 0 siblings, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2010-08-23 18:05 UTC (permalink / raw) To: git; +Cc: Alex Riesen, Christian Couder, Jonathan Nieder, Jeff King My firefox runs in a different "workspace" than what I usually look at while working, and I've beem scratching my head why "git help -w help" doesn't seem to do anything, while I was opening a new tab every time I tried it. And then I tried the command on the Subject line, only to end up with yet another new tab X-<. I know exactly why this happens---we save the config from the command line on a list only so that we can apply them in the correct order after items coming from files, but we do not use the saved values to pass them around to sub-git invocations. 8b1fa77 (Allow passing of configuration parameters in the command line, 2010-03-26) A "trivial fix" would be to pass this info through the execv_git_cmd() interface by either exporting it via an environment variable or by modifying the command line options, but I am not sure about the possible fallouts from such a change. For example, does "git -c var=value config ..." work sensibly when what "config" is told to do (say, remove a section) contradicts with having the named var with a given value? I am wondering if this is worth fixing it in the first place. Opinions? Patches ;-)? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-23 18:05 "git -c web.browser=w3m help -w help" still kicks firefox Junio C Hamano @ 2010-08-23 18:38 ` Jeff King 2010-08-23 19:16 ` Jeff King 2010-08-23 19:02 ` "git -c web.browser=w3m help -w help" still kicks firefox Alex Riesen 1 sibling, 1 reply; 10+ messages in thread From: Jeff King @ 2010-08-23 18:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Alex Riesen, Christian Couder, Jonathan Nieder On Mon, Aug 23, 2010 at 11:05:04AM -0700, Junio C Hamano wrote: > A "trivial fix" would be to pass this info through the execv_git_cmd() > interface by either exporting it via an environment variable or by > modifying the command line options, but I am not sure about the possible > fallouts from such a change. For example, does "git -c var=value config ..." > work sensibly when what "config" is told to do (say, remove a section) > contradicts with having the named var with a given value? > > I am wondering if this is worth fixing it in the first place. IMHO, it needs to be fixed. This bug means "git -c foo=bar X" silently ignores the new value of "foo" if "X" is an external or a shell script. For something like "help" it is a minor inconvenience, but I can certainly see this causing data loss. Just in 30 seconds of grepping, I see that "git -c mergetool.keepbackup=true mergetool" would be silently ignored. Oops. The environment is the only sensible way to pass this down, because we need to hit not just externals, but things like "git config" invocations from shell scripts. IOW, "git -c" really is about executing in a sub-environment that pretends that config is set. Obviously we would need to quote and unquote when using the environment as a transport (or do something horrible like making a temporary config file and pointing at it through the environment). As for "git config", I would assume that "-c" parameters impact how config itself behaves, but have no bearing at all on actual configuration that it writes. I don't know if that is the case now, though. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-23 18:38 ` Jeff King @ 2010-08-23 19:16 ` Jeff King 2010-08-24 6:41 ` [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers Jonathan Nieder 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2010-08-23 19:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Alex Riesen, Christian Couder, Jonathan Nieder On Mon, Aug 23, 2010 at 02:38:57PM -0400, Jeff King wrote: > The environment is the only sensible way to pass this down, because we > need to hit not just externals, but things like "git config" invocations > from shell scripts. IOW, "git -c" really is about executing in a > sub-environment that pretends that config is set. Obviously we would > need to quote and unquote when using the environment as a transport (or > do something horrible like making a temporary config file and pointing > at it through the environment). Here's a first attempt. No idea if it has any bad side effects. :) -- >8 -- Subject: [PATCH] pass "git -c foo=bar" params through environment Git uses the "-c foo=bar" parameters to set a config variable for a single git invocation. We currently do this by making a list in the current process and consulting that list in git_config. This works fine for built-ins, but the config changes are silently ignored by subprocesses, including dashed externals and invocations to "git config" from shell scripts. This patch instead puts them in an environment variable which we consult when looking at config (both internally and via calls "git config"). Signed-off-by: Jeff King <peff@peff.net> --- cache.h | 2 ++ config.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- git.c | 2 +- 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index d8c0a98..b76d7b3 100644 --- a/cache.h +++ b/cache.h @@ -974,7 +974,9 @@ extern int update_server_info(int); 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_parse_ulong(const char *, unsigned long *); diff --git a/config.c b/config.c index 7a18bc9..15eabaf 100644 --- a/config.c +++ b/config.c @@ -8,6 +8,7 @@ #include "cache.h" #include "exec_cmd.h" #include "strbuf.h" +#include "quote.h" #define MAXNAME (256) @@ -34,6 +35,19 @@ static void lowercase(char *p) *p = tolower(*p); } +void git_config_push_parameter(const char *text) +{ + struct strbuf env = STRBUF_INIT; + const char *old = getenv("GIT_CONFIG_PARAMETERS"); + if (old) { + strbuf_addstr(&env, old); + strbuf_addch(&env, ' '); + } + sq_quote_buf(&env, text); + setenv("GIT_CONFIG_PARAMETERS", env.buf, 1); + strbuf_release(&env); +} + int git_config_parse_parameter(const char *text) { struct config_item *ct; @@ -61,6 +75,37 @@ int git_config_parse_parameter(const char *text) return 0; } +int git_config_parse_environment(void) { + const char *env = getenv("GIT_CONFIG_PARAMETERS"); + char *envw; + const char **argv = NULL; + int nr = 0, alloc = 0; + int i; + + if (!env) + return 0; + /* sq_dequote will write over it */ + envw = xstrdup(env); + + if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) { + free(envw); + return error("bogus format in GIT_CONFIG_PARAMETERS"); + } + + for (i = 0; i < nr; i++) { + if (git_config_parse_parameter(argv[i]) < 0) { + error("bogus config parameter: %s", argv[i]); + free(argv); + free(envw); + return -1; + } + } + + free(argv); + free(envw); + return 0; +} + static int get_next_char(void) { int c; @@ -781,7 +826,14 @@ int git_config_global(void) 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; @@ -820,10 +872,9 @@ int git_config(config_fn_t fn, void *data) } free(repo_config); - if (config_parameters) { - ret += git_config_from_parameters(fn, data); + ret += git_config_from_parameters(fn, data); + if (config_parameters) found += 1; - } if (found == 0) return -1; diff --git a/git.c b/git.c index 8dda939..681f96c 100644 --- a/git.c +++ b/git.c @@ -137,7 +137,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) fprintf(stderr, "-c expects a configuration string\n" ); usage(git_usage_string); } - git_config_parse_parameter((*argv)[1]); + git_config_push_parameter((*argv)[1]); (*argv)++; (*argc)--; } else { -- 1.7.2.2.418.g55566.dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers 2010-08-23 19:16 ` Jeff King @ 2010-08-24 6:41 ` Jonathan Nieder 2010-08-24 14:14 ` Jeff King 2010-08-24 19:07 ` [PATCH 2/1] do not pass "git -c foo=bar" " Eric Raible 0 siblings, 2 replies; 10+ messages in thread From: Jonathan Nieder @ 2010-08-24 6:41 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Alex Riesen, Christian Couder Like $GIT_CONFIG, $GIT_CONFIG_PARAMETERS needs to be suppressed by "git push" and its cousins when running local transport helpers to imitate remote transport well. Noticed-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Jeff King wrote: > Here's a first attempt. No idea if it has any bad side effects. :) Here's the transport boundary. cache.h | 3 ++- config.c | 8 ++++---- environment.c | 1 + t/t5400-send-pack.sh | 23 +++++++++++++++++++++++ 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 6f50ea8..181a305 100644 --- a/cache.h +++ b/cache.h @@ -379,6 +379,7 @@ static inline enum object_type object_type(unsigned int mode) #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE" #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR" #define CONFIG_ENVIRONMENT "GIT_CONFIG" +#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS" #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH" #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES" #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS" @@ -397,7 +398,7 @@ static inline enum object_type object_type(unsigned int mode) * environment creation or simple walk of the list. * The number of non-NULL entries is available as a macro. */ -#define LOCAL_REPO_ENV_SIZE 8 +#define LOCAL_REPO_ENV_SIZE 9 extern const char *const local_repo_env[LOCAL_REPO_ENV_SIZE + 1]; extern int is_bare_repository_cfg; diff --git a/config.c b/config.c index e08e32b..c2c995f 100644 --- a/config.c +++ b/config.c @@ -38,13 +38,13 @@ static void lowercase(char *p) void git_config_push_parameter(const char *text) { struct strbuf env = STRBUF_INIT; - const char *old = getenv("GIT_CONFIG_PARAMETERS"); + const char *old = getenv(CONFIG_DATA_ENVIRONMENT); if (old) { strbuf_addstr(&env, old); strbuf_addch(&env, ' '); } sq_quote_buf(&env, text); - setenv("GIT_CONFIG_PARAMETERS", env.buf, 1); + setenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1); strbuf_release(&env); } @@ -76,7 +76,7 @@ int git_config_parse_parameter(const char *text) } int git_config_parse_environment(void) { - const char *env = getenv("GIT_CONFIG_PARAMETERS"); + const char *env = getenv(CONFIG_DATA_ENVIRONMENT); char *envw; const char **argv = NULL; int nr = 0, alloc = 0; @@ -89,7 +89,7 @@ int git_config_parse_environment(void) { if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) { free(envw); - return error("bogus format in GIT_CONFIG_PARAMETERS"); + return error("bogus format in " CONFIG_DATA_ENVIRONMENT); } for (i = 0; i < nr; i++) { diff --git a/environment.c b/environment.c index 83d38d3..a199f63 100644 --- a/environment.c +++ b/environment.c @@ -72,6 +72,7 @@ static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file; const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = { ALTERNATE_DB_ENVIRONMENT, CONFIG_ENVIRONMENT, + CONFIG_DATA_ENVIRONMENT, DB_ENVIRONMENT, GIT_DIR_ENVIRONMENT, GIT_WORK_TREE_ENVIRONMENT, diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh index c718253..5bcf0b8 100755 --- a/t/t5400-send-pack.sh +++ b/t/t5400-send-pack.sh @@ -94,6 +94,29 @@ test_expect_success 'refuse deleting push with denyDeletes' ' test_must_fail git send-pack ./victim :extra master ' +test_expect_success 'cannot override denyDeletes with git -c send-pack' ' + ( + cd victim && + test_might_fail git branch -D extra && + git config receive.denyDeletes true && + git branch extra master + ) && + test_must_fail git -c receive.denyDeletes=false \ + send-pack ./victim :extra master +' + +test_expect_success 'override denyDeletes with git -c receive-pack' ' + ( + cd victim && + test_might_fail git branch -D extra && + git config receive.denyDeletes true && + git branch extra master + ) && + git send-pack \ + --receive-pack="git -c receive.denyDeletes=false receive-pack" \ + ./victim :extra master +' + test_expect_success 'denyNonFastforwards trumps --force' ' ( cd victim && -- 1.7.2.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers 2010-08-24 6:41 ` [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers Jonathan Nieder @ 2010-08-24 14:14 ` Jeff King 2010-08-24 19:07 ` [PATCH 2/1] do not pass "git -c foo=bar" " Eric Raible 1 sibling, 0 replies; 10+ messages in thread From: Jeff King @ 2010-08-24 14:14 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git, Alex Riesen, Christian Couder On Tue, Aug 24, 2010 at 01:41:14AM -0500, Jonathan Nieder wrote: > Like $GIT_CONFIG, $GIT_CONFIG_PARAMETERS needs to be suppressed by > "git push" and its cousins when running local transport helpers to > imitate remote transport well. Thanks, this looks good to me. Though arguably these bits: > +#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS" > [...] > - const char *old = getenv("GIT_CONFIG_PARAMETERS"); > + const char *old = getenv(CONFIG_DATA_ENVIRONMENT); Should be squashed into the original patch. :) -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers 2010-08-24 6:41 ` [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers Jonathan Nieder 2010-08-24 14:14 ` Jeff King @ 2010-08-24 19:07 ` Eric Raible 1 sibling, 0 replies; 10+ messages in thread From: Eric Raible @ 2010-08-24 19:07 UTC (permalink / raw) To: git Jonathan Nieder <jrnieder <at> gmail.com> writes: > #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE" > #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR" > #define CONFIG_ENVIRONMENT "GIT_CONFIG" > +#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS" > #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH" > #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES" > #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS" Given that the pattern is: #define foo_ENVIRONMENT "GIT_foo" Your addition should be: #define CONFIG_PARAMETERS_ENVIRONMENT "GIT_CONFIG_PARAMETERS" Not only that, but the first one should be: #define GRAFT_FILE_ENVIRONMENT "GIT_GRAFT_FILE" ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-23 18:05 "git -c web.browser=w3m help -w help" still kicks firefox Junio C Hamano 2010-08-23 18:38 ` Jeff King @ 2010-08-23 19:02 ` Alex Riesen 2010-08-23 20:33 ` Jeff King 1 sibling, 1 reply; 10+ messages in thread From: Alex Riesen @ 2010-08-23 19:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Nieder, Jeff King On Mon, Aug 23, 2010 at 20:05, Junio C Hamano <gitster@pobox.com> wrote: > I know exactly why this happens---we save the config from the command line > on a list only so that we can apply them in the correct order after items > coming from files, but we do not use the saved values to pass them around > to sub-git invocations. > > 8b1fa77 (Allow passing of configuration parameters in the command line, 2010-03-26) > > A "trivial fix" would be to pass this info through the execv_git_cmd() > interface by either exporting it via an environment variable or by > modifying the command line options, but I am not sure about the possible > fallouts from such a change. For example, does "git -c var=value config ..." > work sensibly when what "config" is told to do (say, remove a section) > contradicts with having the named var with a given value? > > I am wondering if this is worth fixing it in the first place. > > Opinions? Patches ;-)? > Maybe it is worth fixing, but on a case-by-case basis? I mean changing the execv_git_cmd interface (or create a new execv function), so that it can get the list of config vars to pass down to the callee. A trivial case of its use would be to just pass the current config (or, more likely, none). Or, one could give it its own list of config parameters. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-23 19:02 ` "git -c web.browser=w3m help -w help" still kicks firefox Alex Riesen @ 2010-08-23 20:33 ` Jeff King 2010-08-24 5:01 ` Jonathan Nieder 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2010-08-23 20:33 UTC (permalink / raw) To: Alex Riesen; +Cc: Junio C Hamano, git, Christian Couder, Jonathan Nieder On Mon, Aug 23, 2010 at 09:02:36PM +0200, Alex Riesen wrote: > Maybe it is worth fixing, but on a case-by-case basis? > > I mean changing the execv_git_cmd interface (or create a new execv > function), so that it can get the list of config vars to pass down to > the callee. A trivial case of its use would be to just pass the > current config (or, more likely, none). Or, one could give it its own > list of config parameters. I don't think that is enough. We don't necessarily know which config options will be relevant to exec'd processes. We could be running some user-defined command that calls a bunch of other git commands. Or a hook, for that matter. Which does bring up one interesting boundary. If I run: git -c receive.denyDeletes=false git push what should happen? Obviously with cross-server communication the environment won't get passed. I am inclined to say that even for local cases, receive-pack should clear the string. Certainly for the sake of consistency between local and remote transports, but it may also be a security issue (in most cases, no, since you would have to be exec'ing receive-pack directly, and you are clearly already running an arbitrary command, but I can see somebody perhaps crossing a setuid boundary with receive-pack). -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-23 20:33 ` Jeff King @ 2010-08-24 5:01 ` Jonathan Nieder 2010-08-24 14:12 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Nieder @ 2010-08-24 5:01 UTC (permalink / raw) To: Jeff King; +Cc: Alex Riesen, Junio C Hamano, git, Christian Couder Jeff King wrote: > Which does bring up one interesting boundary. If I run: > > git -c receive.denyDeletes=false git push > > what should happen? Obviously with cross-server communication the > environment won't get passed. I am inclined to say that even for local > cases, receive-pack should clear the string. Sticky. I agree with you that that would follow the principle of least surprise. On the other hand if I use git push --receive-pack='git -c receive.denyDeletes=false receive-pack' then I would expect it to work. I don't think this is a security problem because I already could have set the remote $GIT_CONFIG just as easily. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "git -c web.browser=w3m help -w help" still kicks firefox 2010-08-24 5:01 ` Jonathan Nieder @ 2010-08-24 14:12 ` Jeff King 0 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2010-08-24 14:12 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Alex Riesen, Junio C Hamano, git, Christian Couder On Tue, Aug 24, 2010 at 12:01:27AM -0500, Jonathan Nieder wrote: > > Which does bring up one interesting boundary. If I run: > > > > git -c receive.denyDeletes=false git push > > > > what should happen? Obviously with cross-server communication the > > environment won't get passed. I am inclined to say that even for local > > cases, receive-pack should clear the string. > > Sticky. I agree with you that that would follow the principle of > least surprise. > > On the other hand if I use > > git push --receive-pack='git -c receive.denyDeletes=false receive-pack' > > then I would expect it to work. I don't think this is a security > problem because I already could have set the remote $GIT_CONFIG just > as easily. Yeah, I think you are right. Anybody who was trying to cross a setuid boundary with receive-pack is already screwed unless they are cleansing the environment. And I would hope that any such cleansing would be allow-known-good, so the new variable would be blocked along with GIT_CONFIG. So I doubt we are making anything worse, security-wise. I do think we should still remove the variable in the local transport for the sake of least surprise, and I agree that your example above should work. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-08-24 19:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-23 18:05 "git -c web.browser=w3m help -w help" still kicks firefox Junio C Hamano 2010-08-23 18:38 ` Jeff King 2010-08-23 19:16 ` Jeff King 2010-08-24 6:41 ` [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers Jonathan Nieder 2010-08-24 14:14 ` Jeff King 2010-08-24 19:07 ` [PATCH 2/1] do not pass "git -c foo=bar" " Eric Raible 2010-08-23 19:02 ` "git -c web.browser=w3m help -w help" still kicks firefox Alex Riesen 2010-08-23 20:33 ` Jeff King 2010-08-24 5:01 ` Jonathan Nieder 2010-08-24 14:12 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.