{"thread":{"id":"29145","subject":"[PATCH/RFC 1/2] wrapper: supply xsetenv","startedAt":"2011-12-13T12:10:26Z","lastAt":"2011-12-13T19:21:49Z","messageCount":9,"participants":["Erik Faye-Lund","Jeff King","Junio C Hamano","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"181031","messageId":"1323778227-1664-1-git-send-email-kusmabite@gmail.com","threadId":"29145","inReplyTo":null,"subject":"[PATCH/RFC 1/2] wrapper: supply xsetenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-13T12:10:26Z","receivedAt":"2011-12-13T12:10:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed-off-by: Erik Faye-Lund \n---\n git-compat-util.h | 1 +\n wrapper.c | 6 ++++++\n 2 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 77062ed..551a0be 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -439,6 +439,7 @@ extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n+extern int xsetenv(const char *name, const char *val, int override);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..442800b 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -381,3 +381,9 @@ int remove_or_warn(unsigned int mode, const char *file)\n {\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n+\n+int xsetenv(const char *name, const char *val, int overwrite)\n+{\n+\tif (setenv(name, val, overwrite))\n+\t\tdie_errno(\"setenv failed\");\n+}\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181032","messageId":"1323778227-1664-2-git-send-email-kusmabite@gmail.com","threadId":"29145","inReplyTo":"1323778227-1664-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-13T12:10:27Z","receivedAt":"2011-12-13T12:10:27Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This avoids us from accidentally dropping state, possibly leading\nto unexpected behaviour.\n\nSigned-off-by: Erik Faye-Lund \n---\n\nWhile reviewing some patches for Git for Windows, I realized that\nwe almost never check the return-value from setenv. This can lead\nto quite surprising errors in unusual sitations. Mostly, an error\nwould probably be preferred. So here we go.\n\nHowever, I'm not at all convinced myself that all of these make\nsense; in particular settings like GIT_EDITOR and GIT_PAGER could\nperhaps benefit from having a warning printed rather than a hard\nerror.\n\nThoughts?\n\n builtin/clone.c | 2 +-\n builtin/commit.c | 6 +++---\n builtin/help.c | 4 ++--\n builtin/init-db.c | 2 +-\n builtin/merge.c | 4 ++--\n builtin/notes.c | 2 +-\n builtin/remote-ext.c | 4 ++--\n builtin/revert.c | 2 +-\n config.c | 2 +-\n exec_cmd.c | 4 ++--\n git.c | 18 +++++++++---------\n pager.c | 2 +-\n setup.c | 6 +++---\n 13 files changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex efe8b6c..8d81c29 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -566,7 +566,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n-\tsetenv(CONFIG_ENVIRONMENT, mkpath(\"%s/config\", git_dir), 1);\n+\txsetenv(CONFIG_ENVIRONMENT, mkpath(\"%s/config\", git_dir), 1);\n \n \tif (safe_create_leading_directories_const(git_dir) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e36e9ad..2b87da9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -361,13 +361,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n+\t\txsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n \n \t\tif (old_index_env && *old_index_env)\n-\t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n+\t\t\txsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n \t\telse\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n@@ -1023,7 +1023,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (edit_flag)\n \t\tuse_editor = 1;\n \tif (!use_editor)\n-\t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n+\t\txsetenv(\"GIT_EDITOR\", \":\", 1);\n \n \t/* Sanity check options */\n \tif (amend && !current_head)\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 61ff798..e7dc15b 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -330,7 +330,7 @@ static void setup_man_path(void)\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n \n-\tsetenv(\"MANPATH\", new_path.buf, 1);\n+\txsetenv(\"MANPATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n }\n@@ -371,7 +371,7 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\txsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(\"no info viewer handled the request\");\n }\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex d07554c..21ff09e 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -537,7 +537,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \tif (is_bare_repository_cfg == 1) {\n \t\tstatic char git_dir[PATH_MAX+1];\n \n-\t\tsetenv(GIT_DIR_ENVIRONMENT,\n+\t\txsetenv(GIT_DIR_ENVIRONMENT,\n \t\t\tgetcwd(git_dir, sizeof(git_dir)), argc > 0);\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a1c8534..a4ae473 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1257,7 +1257,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstrbuf_addstr(&buf, \"merge\");\n \tfor (i = 0; i < argc; i++)\n \t\tstrbuf_addf(&buf, \" %s\", argv[i]);\n-\tsetenv(\"GIT_REFLOG_ACTION\", buf.buf, 0);\n+\txsetenv(\"GIT_REFLOG_ACTION\", buf.buf, 0);\n \tstrbuf_reset(&buf);\n \n \tfor (i = 0; i < argc; i++) {\n@@ -1267,7 +1267,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tremotes = &commit_list_insert(commit, remotes)->next;\n \t\tstrbuf_addf(&buf, \"GITHEAD_%s\",\n \t\t\t sha1_to_hex(commit->object.sha1));\n-\t\tsetenv(buf.buf, argv[i], 1);\n+\t\txsetenv(buf.buf, argv[i], 1);\n \t\tstrbuf_reset(&buf);\n \t\tif (merge_remote_util(commit) &&\n \t\t merge_remote_util(commit)->obj &&\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 10b8bc7..7b53c69 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -1076,7 +1076,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, override_notes_ref);\n \t\texpand_notes_ref(&sb);\n-\t\tsetenv(\"GIT_NOTES_REF\", sb.buf, 1);\n+\t\txsetenv(\"GIT_NOTES_REF\", sb.buf, 1);\n \t\tstrbuf_release(&sb);\n \t}\n \ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 692c834..0b2169a 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -38,8 +38,8 @@ static char *strip_escapes(const char *str, const char *service,\n \t\tpsoff = 4;\n \n \t/* Pass the service to command. */\n-\tsetenv(\"GIT_EXT_SERVICE\", service, 1);\n-\tsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service + psoff, 1);\n+\txsetenv(\"GIT_EXT_SERVICE\", service, 1);\n+\txsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service + psoff, 1);\n \n \t/* Scan the length of argument. */\n \twhile (str[rpos] && (escape || str[rpos] != ' ')) {\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..955a99f 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -1007,7 +1007,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tstruct commit_list *cur;\n \tint res;\n \n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\txsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\ndiff --git a/config.c b/config.c\nindex 5ea101f..f461a62 100644\n--- a/config.c\n+++ b/config.c\n@@ -43,7 +43,7 @@ void git_config_push_parameter(const char *text)\n \t\tstrbuf_addch(&env, ' ');\n \t}\n \tsq_quote_buf(&env, text);\n-\tsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);\n+\txsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);\n \tstrbuf_release(&env);\n }\n \ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 171e841..a5a92dd 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -63,7 +63,7 @@ void git_set_argv_exec_path(const char *exec_path)\n \t/*\n \t * Propagate this setting to external programs.\n \t */\n-\tsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);\n+\txsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);\n }\n \n \n@@ -108,7 +108,7 @@ void setup_path(void)\n \telse\n \t\tstrbuf_addstr(&new_path, _PATH_DEFPATH);\n \n-\tsetenv(\"PATH\", new_path.buf, 1);\n+\txsetenv(\"PATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n }\ndiff --git a/git.c b/git.c\nindex 8e34903..cb80de2 100644\n--- a/git.c\n+++ b/git.c\n@@ -54,7 +54,7 @@ int check_pager_config(const char *cmd)\n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n \tcase 0:\n-\t\tsetenv(\"GIT_PAGER\", \"cat\", 1);\n+\t\txsetenv(\"GIT_PAGER\", \"cat\", 1);\n \t\tbreak;\n \tcase 1:\n \t\tsetup_pager();\n@@ -109,7 +109,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--no-replace-objects\")) {\n \t\t\tread_replace_refs = 0;\n-\t\t\tsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, \"1\", 1);\n+\t\t\txsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, \"1\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--git-dir\")) {\n@@ -117,13 +117,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for --git-dir.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--namespace\")) {\n@@ -131,13 +131,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No namespace given for --namespace.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--namespace=\")) {\n-\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);\n+\t\t\txsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n@@ -145,19 +145,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for --work-tree.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--work-tree=\")) {\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n+\t\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tis_bare_repository_cfg = 1;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"-c\")) {\ndiff --git a/pager.c b/pager.c\nindex 975955b..d3a1299 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -76,7 +76,7 @@ void setup_pager(void)\n \tif (!pager)\n \t\treturn;\n \n-\tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n+\txsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n \tpager_argv[0] = pager;\ndiff --git a/setup.c b/setup.c\nindex 61c22e6..06f38d0 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -309,7 +309,7 @@ void setup_work_tree(void)\n \t * if $GIT_WORK_TREE is set relative\n \t */\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n-\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n+\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n \tset_git_dir(relative_path(git_dir, work_tree));\n \tinitialized = 1;\n@@ -683,9 +683,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n \tif (prefix)\n-\t\tsetenv(\"GIT_PREFIX\", prefix, 1);\n+\t\txsetenv(\"GIT_PREFIX\", prefix, 1);\n \telse\n-\t\tsetenv(\"GIT_PREFIX\", \"\", 1);\n+\t\txsetenv(\"GIT_PREFIX\", \"\", 1);\n \n \tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181053","messageId":"20111213181602.GB1663@sigill.intra.peff.net","threadId":"29145","inReplyTo":"1323778227-1664-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 1/2] wrapper: supply xsetenv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T18:16:02Z","receivedAt":"2011-12-13T18:16:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 01:10:26PM +0100, Erik Faye-Lund wrote:\n\n> +int xsetenv(const char *name, const char *val, int overwrite)\n> +{\n> +\tif (setenv(name, val, overwrite))\n> +\t\tdie_errno(\"setenv failed\");\n> +}\n\nIt probably doesn't matter, because the error condition is almost\ncertainly just \"oops, we ran out of memory\". But you could also print\nthe name of the variable being set, which may give the user a clue to\nsome misconfiguration (e.g., trying to put some extremely long value\ninto the environment).\n\n-Peff\n"},{"id":"181054","messageId":"20111213181946.GC1663@sigill.intra.peff.net","threadId":"29145","inReplyTo":"1323778227-1664-2-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T18:19:46Z","receivedAt":"2011-12-13T18:19:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:\n\n> While reviewing some patches for Git for Windows, I realized that\n> we almost never check the return-value from setenv. This can lead\n> to quite surprising errors in unusual sitations. Mostly, an error\n> would probably be preferred. So here we go.\n> \n> However, I'm not at all convinced myself that all of these make\n> sense; in particular settings like GIT_EDITOR and GIT_PAGER could\n> perhaps benefit from having a warning printed rather than a hard\n> error.\n> \n> Thoughts?\n\nI wrote almost the same patch once[1], but failed to actually push it\nthrough to acceptance. There weren't any objections, just that nobody\nreally cared. I think it's a reasonable thing to do. The chances of\nsetenv failing are very low, but the consequences could be quite bad.\n\nThere is also a call to putenv in git.c which should be checked (or\ncould arguably just be converted to setenv).\n\n-Peff\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/134466\n"},{"id":"181059","messageId":"7v1us8wccm.fsf@alter.siamese.dyndns.org","threadId":"29145","inReplyTo":"20111213181946.GC1663@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T18:33:29Z","receivedAt":"2011-12-13T18:33:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:\n>\n>> While reviewing some patches for Git for Windows, I realized that\n>> we almost never check the return-value from setenv. This can lead\n>> to quite surprising errors in unusual sitations. Mostly, an error\n>> would probably be preferred. So here we go.\n>> \n>> However, I'm not at all convinced myself that all of these make\n>> sense; in particular settings like GIT_EDITOR and GIT_PAGER could\n>> perhaps benefit from having a warning printed rather than a hard\n>> error.\n>> \n>> Thoughts?\n>\n> I wrote almost the same patch once[1], but failed to actually push it\n> through to acceptance.\n\nIn both cases, the patches are _designed_ to fail to attract any\nattention. Your earlier one had this preamble:\n\n Here is a patch. I still feel a little silly writing this. The chances\n that you will run out of memory doing setenv but _not_ doing any of the\n other git operations seems very low.\n\nwhich rather *loudly* says \"ignore me, please!\" ;-)\n\nErik's patch this round is no better; if its log message said something\nlike \"On platform X, the environment space is merely nKB and setenv has\nmuch higher chance of failing than on typical Linux boxes\", it would have\nbeen a no brainer for me to respond with \"makes perfect sense but don't we\nalso use putenv?\" immediately.\n"},{"id":"181060","messageId":"7vwra0uxqo.fsf@alter.siamese.dyndns.org","threadId":"29145","inReplyTo":"20111213181602.GB1663@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/2] wrapper: supply xsetenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T18:34:23Z","receivedAt":"2011-12-13T18:34:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 13, 2011 at 01:10:26PM +0100, Erik Faye-Lund wrote:\n>\n>> +int xsetenv(const char *name, const char *val, int overwrite)\n>> +{\n>> +\tif (setenv(name, val, overwrite))\n>> +\t\tdie_errno(\"setenv failed\");\n>> +}\n>\n> It probably doesn't matter, because the error condition is almost\n> certainly just \"oops, we ran out of memory\". But you could also print\n> the name of the variable being set, which may give the user a clue to\n> some misconfiguration (e.g., trying to put some extremely long value\n> into the environment).\n\nDo we have enough memory to format that message in that situation ;-)?\n"},{"id":"181061","messageId":"CABPQNSZ_r4u-yXtEGw8duZyoN3SdF5p+7vabz2qqS161kgkHWQ@mail.gmail.com","threadId":"29145","inReplyTo":"7vwra0uxqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] wrapper: supply xsetenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-13T18:52:34Z","receivedAt":"2011-12-13T18:52:34Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Dec 13, 2011 at 7:34 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Tue, Dec 13, 2011 at 01:10:26PM +0100, Erik Faye-Lund wrote:\n>>\n>>> +int xsetenv(const char *name, const char *val, int overwrite)\n>>> +{\n>>> +    if (setenv(name, val, overwrite))\n>>> +            die_errno(\"setenv failed\");\n>>> +}\n>>\n>> It probably doesn't matter, because the error condition is almost\n>> certainly just \"oops, we ran out of memory\". But you could also print\n>> the name of the variable being set, which may give the user a clue to\n>> some misconfiguration (e.g., trying to put some extremely long value\n>> into the environment).\n>\n> Do we have enough memory to format that message in that situation ;-)?\n\nWe could. Running out of environment space is not the same as running\nout of memory. For instance, Windows has a maximum environment size of\n32 kB. Older Linux kernels maxed out at 128 kB.\n\nSo I think it's a good idea to at least try. The worst that can happen\nis another, less descriptive error, no?\n"},{"id":"181062","messageId":"CABPQNSbL2wWFeg8A+H9c2ZzgYaWPMtuYpsPLMp_S-5_tv20cXw@mail.gmail.com","threadId":"29145","inReplyTo":"7v1us8wccm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 2/2] change all unchecked calls to setenv to xsetenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-13T18:55:32Z","receivedAt":"2011-12-13T18:55:32Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Dec 13, 2011 at 7:33 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Tue, Dec 13, 2011 at 01:10:27PM +0100, Erik Faye-Lund wrote:\n>>\n>>> While reviewing some patches for Git for Windows, I realized that\n>>> we almost never check the return-value from setenv. This can lead\n>>> to quite surprising errors in unusual sitations. Mostly, an error\n>>> would probably be preferred. So here we go.\n>>>\n>>> However, I'm not at all convinced myself that all of these make\n>>> sense; in particular settings like GIT_EDITOR and GIT_PAGER could\n>>> perhaps benefit from having a warning printed rather than a hard\n>>> error.\n>>>\n>>> Thoughts?\n>>\n>> I wrote almost the same patch once[1], but failed to actually push it\n>> through to acceptance.\n>\n> In both cases, the patches are _designed_ to fail to attract any\n> attention.  Your earlier one had this preamble:\n>\n>  Here is a patch. I still feel a little silly writing this. The chances\n>  that you will run out of memory doing setenv but _not_ doing any of the\n>  other git operations seems very low.\n>\n> which rather *loudly* says \"ignore me, please!\" ;-)\n>\n> Erik's patch this round is no better; if its log message said something\n> like \"On platform X, the environment space is merely nKB and setenv has\n> much higher chance of failing than on typical Linux boxes\", it would have\n> been a no brainer for me to respond with \"makes perfect sense but don't we\n> also use putenv?\" immediately.\n>\n\nIt could be because I treated this completely like a theoretical\npatch; I haven't seen it actually happen.\n\nBut you are right, Windows 32 kB environment limit makes this much\nmore likely than your average Linux box. So perhaps I should add a\nnotice about that in the next round...\n"},{"id":"181067","messageId":"m2wra0p99u.fsf@igel.home","threadId":"29145","inReplyTo":"CABPQNSZ_r4u-yXtEGw8duZyoN3SdF5p+7vabz2qqS161kgkHWQ@mail.gmail.com","subject":"Re: [PATCH/RFC 1/2] wrapper: supply xsetenv","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-12-13T19:21:49Z","receivedAt":"2011-12-13T19:21:49Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Erik Faye-Lund writes:\n\n> Older Linux kernels maxed out at 128 kB.\n\nBut you wouldn't detect that at setenv time.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}