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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 19, 2018, 18:07 UTC
Message-ID: <20180319110735.56f860f2794c36b1e2e6180c@google.com>
In-Reply-To: <20180303113637.26518-3-pclouds@gmail.com>

On Sat, 3 Mar 2018 18:35:55 +0700 Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 10 quoted lines

> 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 <pclouds@gmail.com>

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

Show 11 quoted lines

> -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.
   */