Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

2014-07-22 Thread Jeff King 
On Mon, Jul 21, 2014 at 07:10:13PM +0200, René Scharfe wrote:

> Probably.  And I was so glad to have found an example case for getcwd
> without dying and without touching the get-there-and-back cases. :) Guess
> I'll have to look closer at setup.c and perhaps unix-socket.c for a
> replacement.

I think just:

  const char *x = xgetcwd();
  setenv(GIT_DIR_ENVIRONMENT, x, 0);
  free(x);

would be enough?

> By the way: Simply setting $GIT_DIR to "." probably won't work in the two
> cases, I guess?

It might, but I'd be a little wary. For example, for the call in
init_db, would we later then chdir to the working tree in order to do a
checkout (since init_db is part of a clone)? Even if it works now, it
seems like a bit of an accident waiting to happen.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

2014-07-21 Thread Junio C Hamano 
Jeff King  writes:

> On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:
>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 56f85e2..c4958b6 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const 
>> char *prefix)
>>  usage(init_db_usage[0]);
>>  }
>>  if (is_bare_repository_cfg == 1) {
>> -static char git_dir[PATH_MAX+1];
>> -
>> -setenv(GIT_DIR_ENVIRONMENT,
>> -getcwd(git_dir, sizeof(git_dir)), argc > 0);
>> +struct strbuf cwd = STRBUF_INIT;
>> +strbuf_getcwd(&cwd);
>> +setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
>> +strbuf_release(&cwd);
>
> Hmm. You are not making anything worse here, as we already do not check
> the return value of getcwd. But what happens if it fails? Looks like we
> currently get a segfault, and the new code will silently set the
> variable to the empty string. Neither is particularly helpful.
>
> Should we be using the xgetcwd helper that you add in the next patch?
>
>> -setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, 
>> sizeof(git_dir)), 0);
>> +strbuf_getcwd(&cwd);
>> +setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
>> +strbuf_release(&cwd);
>
> Ditto here.

Good eyes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

2014-07-21 Thread René Scharfe 

Am 21.07.2014 04:33, schrieb Jeff King:

On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:


diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..c4958b6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
usage(init_db_usage[0]);
}
if (is_bare_repository_cfg == 1) {
-   static char git_dir[PATH_MAX+1];
-
-   setenv(GIT_DIR_ENVIRONMENT,
-   getcwd(git_dir, sizeof(git_dir)), argc > 0);
+   struct strbuf cwd = STRBUF_INIT;
+   strbuf_getcwd(&cwd);
+   setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
+   strbuf_release(&cwd);


Hmm. You are not making anything worse here, as we already do not check
the return value of getcwd. But what happens if it fails? Looks like we
currently get a segfault, and the new code will silently set the
variable to the empty string. Neither is particularly helpful.

Should we be using the xgetcwd helper that you add in the next patch?


Probably.  And I was so glad to have found an example case for getcwd 
without dying and without touching the get-there-and-back cases. :) 
Guess I'll have to look closer at setup.c and perhaps unix-socket.c for 
a replacement.


By the way: Simply setting $GIT_DIR to "." probably won't work in the 
two cases, I guess?





-   setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, 
sizeof(git_dir)), 0);
+   strbuf_getcwd(&cwd);
+   setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
+   strbuf_release(&cwd);


Ditto here.

-Peff


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

2014-07-20 Thread Jeff King 
On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 56f85e2..c4958b6 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>   usage(init_db_usage[0]);
>   }
>   if (is_bare_repository_cfg == 1) {
> - static char git_dir[PATH_MAX+1];
> -
> - setenv(GIT_DIR_ENVIRONMENT,
> - getcwd(git_dir, sizeof(git_dir)), argc > 0);
> + struct strbuf cwd = STRBUF_INIT;
> + strbuf_getcwd(&cwd);
> + setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
> + strbuf_release(&cwd);

Hmm. You are not making anything worse here, as we already do not check
the return value of getcwd. But what happens if it fails? Looks like we
currently get a segfault, and the new code will silently set the
variable to the empty string. Neither is particularly helpful.

Should we be using the xgetcwd helper that you add in the next patch?

> - setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, 
> sizeof(git_dir)), 0);
> + strbuf_getcwd(&cwd);
> + setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
> + strbuf_release(&cwd);

Ditto here.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

2014-07-20 Thread René Scharfe 
Signed-off-by: Rene Scharfe 
---
 builtin/init-db.c | 8 
 git.c | 6 --
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..c4958b6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
usage(init_db_usage[0]);
}
if (is_bare_repository_cfg == 1) {
-   static char git_dir[PATH_MAX+1];
-
-   setenv(GIT_DIR_ENVIRONMENT,
-   getcwd(git_dir, sizeof(git_dir)), argc > 0);
+   struct strbuf cwd = STRBUF_INIT;
+   strbuf_getcwd(&cwd);
+   setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
+   strbuf_release(&cwd);
}
 
if (init_shared_repository != -1)
diff --git a/git.c b/git.c
index 5b6c761..3f52c43 100644
--- a/git.c
+++ b/git.c
@@ -161,9 +161,11 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
-   static char git_dir[PATH_MAX+1];
+   struct strbuf cwd = STRBUF_INIT;
is_bare_repository_cfg = 1;
-   setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, 
sizeof(git_dir)), 0);
+   strbuf_getcwd(&cwd);
+   setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
+   strbuf_release(&cwd);
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
-- 
2.0.2


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html