On Thu, Nov 11, 2010 at 12:17:28PM -0600, Jonathan Nieder wrote:
> Kirill Smelkov wrote:
>
> > getenv(3) returns not-permanent buffer which may be changed by e.g.
> > putenv(3) call (*).
>
> Yikes. Thanks for the example.
Nevermind. However it was not so fun to debug :)
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -88,6 +88,7 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
> > static void setup_git_env(void)
> > {
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > + git_dir = git_dir ? xstrdup(git_dir) : NULL;
> > if (!git_dir) {
> > git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> > git_dir = git_dir ? xstrdup(git_dir) : NULL;
>
> Maybe we can avoid (some) repetition like this?
>
> diff --git a/environment.c b/environment.c
> index de5581f..942f1e4 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -87,25 +87,31 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
> static void setup_git_env(void)
> {
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir) {
> - git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> - git_dir = git_dir ? xstrdup(git_dir) : NULL;
> - }
> if (!git_dir)
> + git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> + if (git_dir)
> + git_dir = xstrdup(git_dir);
> + else
> git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> +
> git_object_dir = getenv(DB_ENVIRONMENT);
> - if (!git_object_dir) {
> - git_object_dir = xmalloc(strlen(git_dir) + 9);
> - sprintf(git_object_dir, "%s/objects", git_dir);
> - }
> + if (git_object_dir)
> + git_object_dir = xstrdup(git_object_dir);
> + else
> + git_object_dir = git_pathdup("objects");
> +
> git_index_file = getenv(INDEX_ENVIRONMENT);
> - if (!git_index_file) {
> - git_index_file = xmalloc(strlen(git_dir) + 7);
> - sprintf(git_index_file, "%s/index", git_dir);
> - }
> + if (git_index_file)
> + git_index_file = xstrdup(git_index_file);
> + else
> + git_index_file = git_pathdup("index");
> +
> git_graft_file = getenv(GRAFT_ENVIRONMENT);
> - if (!git_graft_file)
> + if (git_graft_file)
> + git_graft_file = xstrdup(git_graft_file);
> + else
> git_graft_file = git_pathdup("info/grafts");
> +
To me it gets hairy and we don't cover all and even most getenv cases.
Look e.g. in commit.c:
static void determine_author_info(void)
{
char *name, *email, *date;
name = getenv("GIT_AUTHOR_NAME");
email = getenv("GIT_AUTHOR_EMAIL");
date = getenv("GIT_AUTHOR_DATE");
/* ... */
if (signoff) {
struct strbuf sob = STRBUF_INIT;
int i;
strbuf_addstr(&sob, sign_off_header);
strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL")));
/* ... */
notes.c:
struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
{
struct notes_rewrite_cfg *c = xmalloc(sizeof(struct notes_rewrite_cfg));
const char *rewrite_mode_env = getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT);
const char *rewrite_refs_env = getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT);
editor.c:
const char *git_editor(void)
{
const char *editor = getenv("GIT_EDITOR");
const char *terminal = getenv("TERM");
http-backend.c:
static void run_service(const char **argv)
{
const char *encoding = getenv("HTTP_CONTENT_ENCODING");
const char *user = getenv("REMOTE_USER");
const char *host = getenv("REMOTE_ADDR");
etc...
To me, it's very unfortunate that subsequent getenv() could overwrite
previous getenv() result, but according to `man 3 getenv` all these
places are buggy.
Maybe we'll need something like our own xgetenv() which will keep vars
in some kind of hash tab so that get/put on other vars do not interfere
with what was originally returned by xgetenv().
I don't know.
Unfortunately I can't afford myself to dive into all this, so please
choose what you like more.
Thanks,
Kirill
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html