Re: [PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Duy Nguyen]

On Tue, Feb 27, 2018 at 1:57 AM, Eric Sunshine  wrote:
> Can:
>
> char *old_gitdir = repo->gitdir;
> repo->gitdir = xstrdup(...);
> free(old_gitdir);
>
> be simplified to:
>
> free(repo->gitdir);
> repo->gitdir = xstrdup(...);
>
> ?

No because I think "root" (not quoted here) could point to the same
value as repo->gitdir, if you free that first, the second xstrdup()
will duplicate a freed memory. See 1fb2b636c6 (set_git_dir: handle
feeding gitdir to itself - 2017-09-05)

I will add a comment about this since it's not very clear from this code.
-- 
Duy

Re: [PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Duy Nguyen]

On Tue, Feb 27, 2018 at 1:57 AM, Eric Sunshine  wrote:

>> +   repo_set_commondir(repo, o->shared_root);
>
> The repo_set_gitdir() prototype (below) makes it seem as if the last
> argument ('o', in this case) is optional, presumably by passing in
> NULL, however, this code does not appear to be prepared to deal with
> NULL.

I know. I went this this struct because I really hate to write lots of
NULL without names

repo_set_gitdir(gitdir, NULL, NULL, NULL, ...);

but I could not find a nicer way to make the whole struct optional,
not without repeating more code or using preprocessor. Given that this
function has 3-4 call sites max, I think it's still ok.

>
>> +   expand_base_dir(>objects.objectdir, o->object_dir,
>> +   repo->commondir, "objects");
>> +   expand_base_dir(>graft_file, o->graft_file,
>> +   repo->commondir, "info/grafts");
>> +   expand_base_dir(>index_file, o->index_file,
>> +   repo->gitdir, "index");
>>  }
-- 
Duy

Re: [PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Duy Nguyen]

On Tue, Feb 27, 2018 at 3:30 AM, Stefan Beller  wrote:
>> diff --git a/setup.c b/setup.c
>> index c5d55dcee4..6fac1bb58a 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -1116,8 +1116,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>> const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
>> if (!gitdir)
>> gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
>> -   repo_set_gitdir(the_repository, gitdir);
>> -   setup_git_env();
>> +   setup_git_env(gitdir);
>> }
>
> What makes git_dir special, such that we have to pass it in here?
> Could the check for getenv(GIT_DIR_ENVIRONMENT) and fallback to
> DEFAULT_... be part of setup_git_env() ?
> Oh I guess that cannot be done easily as the submodule code
> spcifically doesn't want to discover the git dir itself.

No, submodule code already does not care about setup_git_env().

Back to your first question, this is related to the "NEEDSWORK"
comment in this code. We _should_ always set_git_dir() before leaving
setup_git_directory_gently() then everybody behaves the same way and
we don't need this special setup_git_env() here at all. BUT we need to
be careful about setting environment variable $GIT_DIR, done inside
set_git_dir(), because sub-processes will inherit it and if we're not
careful, we'll break .git discovery in those. There's another thing I
don't like about this is setup_work_tree() using set_git_dir() the
second time to adjust relative $GIT_DIR and friends when it moves
$CWD. I'll need to fix this, soon.

So in short, it's special because the current situation is messy and
(hopefully) temporary. But it should eventually be gone.
-- 
Duy

Re: [PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Stefan Beller]

> diff --git a/setup.c b/setup.c
> index c5d55dcee4..6fac1bb58a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1116,8 +1116,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
> const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
> if (!gitdir)
> gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
> -   repo_set_gitdir(the_repository, gitdir);
> -   setup_git_env();
> +   setup_git_env(gitdir);
> }

What makes git_dir special, such that we have to pass it in here?
Could the check for getenv(GIT_DIR_ENVIRONMENT) and fallback to
DEFAULT_... be part of setup_git_env() ?
Oh I guess that cannot be done easily as the submodule code
spcifically doesn't want to discover the git dir itself.


> if (startup_info->have_repository)
> repo_set_hash_algo(the_repository, 
> repo_fmt.hash_algo);
> --
> 2.16.1.435.g8f24da2e1a
>

Re: [PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Eric Sunshine]

On Mon, Feb 26, 2018 at 5:30 AM, Nguyễn Thái Ngọc Duy  wrote:
> It does not make sense that generic repository code contains handling
> of environment variables, which are specific for the main repository
> only. Refactor repo_set_gitdir() function to take $GIT_DIR and
> optionally _all_ other customizable paths. These optional paths can be
> NULL and will be calculated according to the default directory layout.
>
> Note that some dead functions are left behind to reduce diff
> noise. They will be deleted in the next patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/environment.c b/environment.c
> @@ -148,10 +148,17 @@ static char *expand_namespace(const char *raw_namespace)
> -void setup_git_env(void)
> +void setup_git_env(const char *git_dir)
>  {
> const char *shallow_file;
> const char *replace_ref_base;
> +   struct set_gitdir_args args = { NULL };
> +
> +   args.shared_root = getenv(GIT_COMMON_DIR_ENVIRONMENT);
> +   args.object_dir = getenv(DB_ENVIRONMENT);
> +   args.graft_file = getenv(GRAFT_ENVIRONMENT);
> +   args.index_file = getenv(INDEX_ENVIRONMENT);

According to POSIX[1], the result of getenv() may be invalidated by
another call to getenv() (or setenv() or unsetenv() or putenv()).

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html

> +   repo_set_gitdir(the_repository, git_dir, );
> diff --git a/repository.c b/repository.c
> @@ -61,15 +61,50 @@ static void repo_setup_env(struct repository *repo)
> +void repo_set_gitdir(struct repository *repo,
> +const char *root,
> +const struct set_gitdir_args *o)
> +{
> +   const char *gitfile = read_gitfile(root);
> +   char *old_gitdir = repo->gitdir;
> +
> +   repo->gitdir = xstrdup(gitfile ? gitfile : root);
> free(old_gitdir);

Can:

char *old_gitdir = repo->gitdir;
repo->gitdir = xstrdup(...);
free(old_gitdir);

be simplified to:

free(repo->gitdir);
repo->gitdir = xstrdup(...);

?

> +   repo_set_commondir(repo, o->shared_root);

The repo_set_gitdir() prototype (below) makes it seem as if the last
argument ('o', in this case) is optional, presumably by passing in
NULL, however, this code does not appear to be prepared to deal with
NULL.

> +   expand_base_dir(>objects.objectdir, o->object_dir,
> +   repo->commondir, "objects");
> +   expand_base_dir(>graft_file, o->graft_file,
> +   repo->commondir, "info/grafts");
> +   expand_base_dir(>index_file, o->index_file,
> +   repo->gitdir, "index");
>  }
> diff --git a/repository.h b/repository.h
> @@ -89,7 +89,16 @@ struct repository {
> +struct set_gitdir_args {
> +   [...]
> +};
> +
> +extern void repo_set_gitdir(struct repository *repo,
> +   const char *root,
> +   const struct set_gitdir_args *optional);

[PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Nguyễn Thái Ngọc Duy]

It does not make sense that generic repository code contains handling
of environment variables, which are specific for the main repository
only. Refactor repo_set_gitdir() function to take $GIT_DIR and
optionally _all_ other customizable paths. These optional paths can be
NULL and will be calculated according to the default directory layout.

Note that some dead functions are left behind to reduce diff
noise. They will be deleted in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h   |  2 +-
 environment.c | 12 +---
 repository.c  | 48 ++--
 repository.h  | 11 ++-
 setup.c   |  3 +--
 5 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 5717399183..b164a407eb 100644
--- a/cache.h
+++ b/cache.h
@@ -459,7 +459,7 @@ static inline enum object_type object_type(unsigned int 
mode)
  */
 extern const char * const local_repo_env[];
 
-extern void setup_git_env(void);
+extern void setup_git_env(const char *git_dir);
 
 /*
  * Returns true iff we have a configured git repository (either via
diff --git a/environment.c b/environment.c
index ec10b062e6..2905042b16 100644
--- a/environment.c
+++ b/environment.c
@@ -148,10 +148,17 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
-void setup_git_env(void)
+void setup_git_env(const char *git_dir)
 {
const char *shallow_file;
const char *replace_ref_base;
+   struct set_gitdir_args args = { NULL };
+
+   args.shared_root = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+   args.object_dir = getenv(DB_ENVIRONMENT);
+   args.graft_file = getenv(GRAFT_ENVIRONMENT);
+   args.index_file = getenv(INDEX_ENVIRONMENT);
+   repo_set_gitdir(the_repository, git_dir, );
 
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
check_replace_refs = 0;
@@ -301,8 +308,7 @@ int set_git_dir(const char *path)
 {
if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
return error("Could not set GIT_DIR to '%s'", path);
-   repo_set_gitdir(the_repository, path);
-   setup_git_env();
+   setup_git_env(path);
return 0;
 }
 
diff --git a/repository.c b/repository.c
index a069b1b640..70dc8dc661 100644
--- a/repository.c
+++ b/repository.c
@@ -61,15 +61,50 @@ static void repo_setup_env(struct repository *repo)
 "index", !repo->ignore_env);
 }
 
-void repo_set_gitdir(struct repository *repo, const char *path)
+static void expand_base_dir(char **out, const char *in,
+   const char *base_dir, const char *def_in)
 {
-   const char *gitfile = read_gitfile(path);
-   char *old_gitdir = repo->gitdir;
+   free(*out);
+   if (in)
+   *out = xstrdup(in);
+   else
+   *out = xstrfmt("%s/%s", base_dir, def_in);
+}
+
+static void repo_set_commondir(struct repository *repo,
+  const char *shared_root)
+{
+   struct strbuf sb = STRBUF_INIT;
+
+   free(repo->commondir);
 
-   repo->gitdir = xstrdup(gitfile ? gitfile : path);
-   repo_setup_env(repo);
+   if (shared_root) {
+   repo->different_commondir = 1;
+   repo->commondir = xstrdup(shared_root);
+   return;
+   }
 
+   repo->different_commondir = get_common_dir_noenv(, repo->gitdir);
+   repo->commondir = strbuf_detach(, NULL);
+}
+
+void repo_set_gitdir(struct repository *repo,
+const char *root,
+const struct set_gitdir_args *o)
+{
+   const char *gitfile = read_gitfile(root);
+   char *old_gitdir = repo->gitdir;
+
+   repo->gitdir = xstrdup(gitfile ? gitfile : root);
free(old_gitdir);
+
+   repo_set_commondir(repo, o->shared_root);
+   expand_base_dir(>objects.objectdir, o->object_dir,
+   repo->commondir, "objects");
+   expand_base_dir(>graft_file, o->graft_file,
+   repo->commondir, "info/grafts");
+   expand_base_dir(>index_file, o->index_file,
+   repo->gitdir, "index");
 }
 
 void repo_set_hash_algo(struct repository *repo, int hash_algo)
@@ -87,6 +122,7 @@ static int repo_init_gitdir(struct repository *repo, const 
char *gitdir)
int error = 0;
char *abspath = NULL;
const char *resolved_gitdir;
+   struct set_gitdir_args args = { NULL };
 
abspath = real_pathdup(gitdir, 0);
if (!abspath) {
@@ -101,7 +137,7 @@ static int repo_init_gitdir(struct repository *repo, const 
char *gitdir)
goto out;
}
 
-   repo_set_gitdir(repo, resolved_gitdir);
+   repo_set_gitdir(repo, resolved_gitdir, );
 
 out:
free(abspath);
diff --git a/repository.h b/repository.h
index fa73ab8e93..b5b5d138aa 100644
--- a/repository.h
+++ b/repository.h
@@ -89,7 +89,16 @@ struct repository {
 
 extern