Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting

[Justin Tobler <jltobler@gmail.com>]

On 26/06/05 10:00PM, K Jayatheerth wrote:
> Scripts often need to locate the `.git` directory. While `git rev-parse`
> provides this, it relies on command-line flags to dictate path formatting.
> 
> Introduce `path.gitdir.absolute` and `path.gitdir.relative` keys to
> `git repo info`. Exposing separate format-specific keys instead of a base
> `path.gitdir` key avoids default fallbacks and requires callers to state
> their format requirements explicitly. Both keys use `format_path()` to
> resolve paths.

Makes sense.

> To test these keys, introduce the `test_repo_info_path` helper in
> `t/t1900-repo-info.sh`. The helper evaluates paths dynamically and accepts
> environment variable prefixes. This prepares the test suite for future path
> keys that depend on environment overrides, such as `commondir`.
> 
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
>  Documentation/git-repo.adoc |  6 ++++++
>  builtin/repo.c              | 26 ++++++++++++++++++++++++++
>  t/t1900-repo-info.sh        | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index 42262c1983..a0dca7ce88 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -104,6 +104,12 @@ values that they return:
>  `object.format`::
>  	The object format (hash algorithm) used in the repository.
>  
> +`path.gitdir.absolute`::
> +	The canonical absolute path to the Git repository directory (the `.git` directory).
> +
> +`path.gitdir.relative`::
> +	The path to the Git repository directory relative to the current working directory.
> +
>  `references.format`::
>  	The reference storage format. The valid values are:
>  +
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 71a5c1c29c..6e97f6a0e4 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -7,12 +7,14 @@
>  #include "hex.h"
>  #include "odb.h"
>  #include "parse-options.h"
> +#include "path.h"
>  #include "path-walk.h"
>  #include "progress.h"
>  #include "quote.h"
>  #include "ref-filter.h"
>  #include "refs.h"
>  #include "revision.h"
> +#include "setup.h"
>  #include "strbuf.h"
>  #include "string-list.h"
>  #include "shallow.h"
> @@ -75,6 +77,28 @@ static int get_object_format(struct repository *repo, struct strbuf *buf)
>  	return 0;
>  }
>  
> +static int get_path_gitdir_absolute(struct repository *repo, struct strbuf *buf)
> +{
> +	const char *git_dir = repo_get_git_dir(repo);
> +
> +	if (!git_dir)
> +		return error(_("unable to get git directory"));
> +
> +	format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_CANONICAL);

For absolute paths, I don't think we actually need the prefix, but
providing it doesn't probably matter too much either way.

> +	return 0;
> +}
> +
> +static int get_path_gitdir_relative(struct repository *repo, struct strbuf *buf)
> +{
> +	const char *git_dir = repo_get_git_dir(repo);
> +
> +	if (!git_dir)
> +		return error(_("unable to get git directory"));
> +
> +	format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_RELATIVE);
> +	return 0;
> +}

Looks good.

> +
>  static int get_references_format(struct repository *repo, struct strbuf *buf)
>  {
>  	strbuf_addstr(buf,
> @@ -87,6 +111,8 @@ static const struct repo_info_field repo_info_field[] = {
>  	{ "layout.bare", get_layout_bare },
>  	{ "layout.shallow", get_layout_shallow },
>  	{ "object.format", get_object_format },
> +	{ "path.gitdir.absolute", get_path_gitdir_absolute },
> +	{ "path.gitdir.relative", get_path_gitdir_relative },
>  	{ "references.format", get_references_format },
>  };
>  
> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index 39bb77dda0..0660b00bbc 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -155,4 +155,37 @@ test_expect_success 'git repo info -h shows only repo info usage' '
>  	test_grep ! "git repo structure" actual
>  '
>  
> +test_repo_info_path () {
> +	field_name=$1
> +	expect_absolute_eval=$2
> +	expect_relative=$3
> +	env_prefix=$4

nit: I was a bit uncertain regarding the purpose of env_prefix here.
Since the env_prefix is not used by any tests yet, I wonder if it we
should delay adding it until the next patch. If we want to reduce churn
though, I think we could also swap the order of patch 3 and 4.

> +
> +	test_expect_success "query individual key: path.$field_name.absolute${env_prefix:+ ($env_prefix)}" '
> +		(
> +			cd test-repo/sub &&
> +			expect_absolute=$(eval "$expect_absolute_eval") &&

Can we just compute `expect_absolute` prior to passing it instead of
using eval here?

> +			echo "path.$field_name.absolute=$expect_absolute" >expect &&
> +			eval "${env_prefix:+$env_prefix }git repo info \"path.$field_name.absolute\"" >actual &&
> +			test_cmp expect actual
> +		)
> +	'
> +
> +	test_expect_success "query individual key: path.$field_name.relative${env_prefix:+ ($env_prefix)}" '
> +		(
> +			cd test-repo/sub &&
> +			echo "path.$field_name.relative=$expect_relative" >expect &&
> +			eval "${env_prefix:+$env_prefix }git repo info \"path.$field_name.relative\"" >actual &&
> +			test_cmp expect actual
> +		)
> +	'
> +}
> +
> +test_expect_success 'setup test repository layout for path fields' '
> +	git init test-repo &&
> +	mkdir -p test-repo/sub
> +'
> +
> +test_repo_info_path 'gitdir' 'echo "$(cd .. && pwd)/.git"' '../.git'

hmmm, do we expect the path suffix to be the same between relative and
absolute paths for all test cases? If so, we could just have a single
`expect_path_suffix` argument and let the helper compute the appropriate
absolute and relative paths internally.

-Justin