Re: [PATCH v4 05/27] Make git_path() aware of file relocation in $GIT_DIR

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 3, 2014, 01:34 UTC
Message-ID: <CAPig+cTU8-sBsbHpJXHxW2RuXdArWnamTXWvkw9d0WnYpYJtig@mail.gmail.com>
In-Reply-To: <1393675983-3232-6-git-send-email-pclouds@gmail.com>

On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> We allow the user to relocate certain paths out of $GIT_DIR via
> environment variables, e.g. GIT_OBJECT_DIRECTORY, GIT_INDEX_FILE and
> GIT_GRAFT_FILE. All callers are not supposed to use git_path() or

"All callers are not" is unusually difficult to understand. Changing it to "Callers are not" simplifies.

> git_pathdup() to get those paths. Instead they must use
> get_object_directory(), get_index_file() and get_graft_file()
> respectively. This is inconvenient and could be missed in review
> (there's git_path("objects/info/alternates") somewhere in

"(for example, there's..." reads a bit better.

Show 5 quoted lines

> sha1_file.c).
>
> This patch makes git_path() and git_pathdup() understand those
> environment variables. So if you set GIT_OBJECT_DIRECTORY to /foo/bar,
> git_path("objects/abc") should return /tmp/bar/abc. The same is done

I guess you mean it should return /foo/bar/abc.

Show 26 quoted lines

> for the two remaining env variables.
>
> "git rev-parse --git-path" is the wrapper for script use.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-rev-parse.txt |  5 +++++
>  builtin/rev-parse.c             |  7 +++++++
>  cache.h                         |  1 +
>  environment.c                   |  9 ++++++--
>  path.c                          | 46 +++++++++++++++++++++++++++++++++++++++++
>  t/t0060-path-utils.sh           | 19 +++++++++++++++++
>  6 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 0d2cdcd..33e4e90 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -232,6 +232,11 @@ print a message to stderr and exit with nonzero status.
>         repository.  If <path> is a gitfile then the resolved path
>         to the real repository is printed.
>
> +--git-path <path>::
> +       Resolve "$GIT_DIR/<path>" and takes other path relocation
> +       variables such as $GIT_OBJECT_DIRECTORY,
> +       $GIT_INDEX_FILE... into account.

Would it help to add a quick illustration here?

    For example, if GIT_OBJECT_DIRECTORY is /foo/bar,
    then "git rev-parse --git-path objects/abc" returns /foo/bar/abc.

Show 16 quoted lines

>  --show-cdup::
>         When the command is invoked from a subdirectory, show the
>         path of the top-level directory relative to the current
> diff --git a/path.c b/path.c
> index ccd7228..e020530 100644
> --- a/path.c
> +++ b/path.c
> @@ -60,13 +60,59 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>         return cleanup_path(buf);
>  }
>
>  static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
>  {
>         const char *git_dir = get_git_dir();
> +       int gitdir_len;
>         strbuf_addstr(buf, git_dir);

Maybe simplify by dropping git_dir and invoking strbuf_addstr(buf, get_git_dir())?

Show 8 quoted lines

>         if (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
>                 strbuf_addch(buf, '/');
> +       gitdir_len = buf->len;
>         strbuf_vaddf(buf, fmt, args);
> +       adjust_git_path(buf, gitdir_len);
>         strbuf_cleanup_path(buf);
>  }
>