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

[Brandon Williams]

On 03/19, Duy Nguyen wrote:
> On Mon, Mar 19, 2018 at 7:07 PM, Jonathan Tan  
> wrote:
> >> -extern void repo_set_gitdir(struct repository *repo, const char *path);
> >> +struct set_gitdir_args {
> >> + const char *commondir;
> >> + const char *object_dir;
> >> + const char *graft_file;
> >> + const char *index_file;
> >> +};
> >> +
> >> +extern void repo_set_gitdir(struct repository *repo,
> >> + const char *root,
> >> + const struct set_gitdir_args *optional);
> >
> > Optional: Reading this header file alone makes me think that the 3rd
> > argument can be NULL, but it actually can't. I would name it
> > "extra_args" and add a comment inside "struct set_gitdir_args"
> > explaining (e.g.):
> >
> >   /*
> >* Any of these fields may be NULL. If so, the name of that directory
> >* is instead derived from the root path of the repository.
> >*/
> 
> Yeah. I think Eric made the same comment. I'm still (very slowly) in
> the process of unifying the repo setup for the main repo and
> submodules, which hopefully may kill this function or replace it with
> something better. But it's too early to tell. Since this part of the
> series has landed in 'next', I'll post a fixup patch soon with your
> suggestion.
> -- 
> Duy

Yeah looking at the patch this is probably my only complaint.  The rest
looked good.

-- 
Brandon Williams

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

[Duy Nguyen]

On Mon, Mar 19, 2018 at 7:07 PM, Jonathan Tan  wrote:
>> -extern void repo_set_gitdir(struct repository *repo, const char *path);
>> +struct set_gitdir_args {
>> + const char *commondir;
>> + const char *object_dir;
>> + const char *graft_file;
>> + const char *index_file;
>> +};
>> +
>> +extern void repo_set_gitdir(struct repository *repo,
>> + const char *root,
>> + const struct set_gitdir_args *optional);
>
> Optional: Reading this header file alone makes me think that the 3rd
> argument can be NULL, but it actually can't. I would name it
> "extra_args" and add a comment inside "struct set_gitdir_args"
> explaining (e.g.):
>
>   /*
>* Any of these fields may be NULL. If so, the name of that directory
>* is instead derived from the root path of the repository.
>*/

Yeah. I think Eric made the same comment. I'm still (very slowly) in
the process of unifying the repo setup for the main repo and
submodules, which hopefully may kill this function or replace it with
something better. But it's too early to tell. Since this part of the
series has landed in 'next', I'll post a fixup patch soon with your
suggestion.
-- 
Duy

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

[Jonathan Tan]

On Sat,  3 Mar 2018 18:35:55 +0700
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 

Thanks - I've reviewed up to this patch, and patches 1 and 2 look good.

> -extern void repo_set_gitdir(struct repository *repo, const char *path);
> +struct set_gitdir_args {
> + const char *commondir;
> + const char *object_dir;
> + const char *graft_file;
> + const char *index_file;
> +};
> +
> +extern void repo_set_gitdir(struct repository *repo,
> + const char *root,
> + const struct set_gitdir_args *optional);

Optional: Reading this header file alone makes me think that the 3rd
argument can be NULL, but it actually can't. I would name it
"extra_args" and add a comment inside "struct set_gitdir_args"
explaining (e.g.):

  /*
   * Any of these fields may be NULL. If so, the name of that directory
   * is instead derived from the root path of the repository.
   */

[PATCH 02/44] 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 | 30 +++---
 repository.c  | 58 +++
 repository.h  | 11 +-
 setup.c   |  3 +--
 5 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/cache.h b/cache.h
index 9cac7bb518..41ba67cc16 100644
--- a/cache.h
+++ b/cache.h
@@ -484,7 +484,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 de8431e01e..da3f7daa09 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 #include "commit.h"
+#include "argv-array.h"
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
@@ -147,10 +148,34 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
-void setup_git_env(void)
+/*
+ * Wrapper of getenv() that returns a strdup value. This value is kept
+ * in argv to be freed later.
+ */
+static const char *getenv_safe(struct argv_array *argv, const char *name)
+{
+   const char *value = getenv(name);
+
+   if (!value)
+   return NULL;
+
+   argv_array_push(argv, value);
+   return argv->argv[argv->argc - 1];
+}
+
+void setup_git_env(const char *git_dir)
 {
const char *shallow_file;
const char *replace_ref_base;
+   struct set_gitdir_args args = { NULL };
+   struct argv_array to_free = ARGV_ARRAY_INIT;
+
+   args.commondir = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
+   args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
+   args.graft_file = getenv_safe(_free, GRAFT_ENVIRONMENT);
+   args.index_file = getenv_safe(_free, INDEX_ENVIRONMENT);
+   repo_set_gitdir(the_repository, git_dir, );
+   argv_array_clear(_free);
 
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
check_replace_refs = 0;
@@ -300,8 +325,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 0eddf28fcd..bb53b54b6d 100644
--- a/repository.c
+++ b/repository.c
@@ -40,34 +40,55 @@ static int find_common_dir(struct strbuf *sb, const char 
*gitdir, int fromenv)
return get_common_dir_noenv(sb, gitdir);
 }
 
-static void repo_setup_env(struct repository *repo)
+static void expand_base_dir(char **out, const char *in,
+   const char *base_dir, const char *def_in)
+{
+   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 *commondir)
 {
struct strbuf sb = STRBUF_INIT;
 
-   repo->different_commondir = find_common_dir(, repo->gitdir,
-   !repo->ignore_env);
free(repo->commondir);
+
+   if (commondir) {
+   repo->different_commondir = 1;
+   repo->commondir = xstrdup(commondir);
+   return;
+   }
+
+   repo->different_commondir = get_common_dir_noenv(, repo->gitdir);
repo->commondir = strbuf_detach(, NULL);
-   free(repo->objectdir);
-   repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
-   "objects", !repo->ignore_env);
-   free(repo->graft_file);
-   repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
-"info/grafts", !repo->ignore_env);
-   free(repo->index_file);
-   repo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,
-"index", !repo->ignore_env);
 }
 
-void repo_set_gitdir(struct repository *repo, const char *path)
+void repo_set_gitdir(struct repository *repo,
+const char *root,
+const struct set_gitdir_args *o)
 {
-   const char *gitfile = read_gitfile(path);
+   const char *gitfile =