Re: [PATCH 2/3] var: add attributes files locations

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> -static const char *git_etc_gitattributes(void)
> +const char *git_etc_gitattributes(void)
>  {
>  	static const char *system_wide;
>  	if (!system_wide)
> @@ -878,7 +878,7 @@ static const char *git_etc_gitattributes(void)
>  	return system_wide;
>  }
>  
> -static const char *get_home_gitattributes(void)
> +const char *get_home_gitattributes(void)
>  {
>  	if (!git_attributes_file)
>  		git_attributes_file = xdg_config_home("attributes");
> @@ -886,7 +886,7 @@ static const char *get_home_gitattributes(void)
>  	return git_attributes_file;
>  }

These are sensible, but ...

> -static int git_attr_system(void)
> +int git_attr_system(void)
>  {
>  	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
>  }

... even though this is not exactly a new problem, but this function
is misnamed and exposing it outside the file is not exactly nice.
We would want to rename it to perhaps git_attr_use_system() or
something?  "allow" would also work as a verb there, but the point
is without any verb, it is easily confused with what the function
git_etc_git_attributes() wants to do.

side note: arguably the "etc-gitattributes" and "home-gitattributes"
are also suboptimal public names to give these functions, even
though they are fine as long as they are confined in a subsystem.
For global functions, it would be better to give name that match
what the end-users think of them, when possible.  The former would
be "git_attr_system()" and the latter would be "git_attr_global()".

>  struct git_var {
>  	const char *name;
>  	const char *(*read)(int);
> +	int free;
>  };
>  static struct git_var git_vars[] = {
> -	{ "GIT_COMMITTER_IDENT", git_committer_info },
> -	{ "GIT_AUTHOR_IDENT",   git_author_info },
> -	{ "GIT_EDITOR", editor },
> -	{ "GIT_SEQUENCE_EDITOR", sequence_editor },
> -	{ "GIT_PAGER", pager },
> -	{ "GIT_DEFAULT_BRANCH", default_branch },
> -	{ "GIT_SHELL_PATH", shell_path },
> +	{ "GIT_COMMITTER_IDENT", git_committer_info, 0 },
> +	{ "GIT_AUTHOR_IDENT",   git_author_info, 0 },
> +	{ "GIT_EDITOR", editor, 0 },
> +	{ "GIT_SEQUENCE_EDITOR", sequence_editor, 0 },
> +	{ "GIT_PAGER", pager, 0 },
> +	{ "GIT_DEFAULT_BRANCH", default_branch, 0 },
> +	{ "GIT_SHELL_PATH", shell_path, 0 },
> +	{ "GIT_ATTR_SYSTEM", git_attr_val_system, 1 },
> +	{ "GIT_ATTR_GLOBAL", git_attr_val_global, 1 },
>  	{ "", NULL },
>  };

I am not sure if the "free" (which stands for "allocated") is worth
adding here for such a single-use-and-then-exit command.  It is a
maintenance burden for anybody who adds new variables.

Either mark these values with UNLEAK(), or make the ones that do
not allocate xstrdup() so that they can be free'd blindly, would be
less unwieldy option, I guess.

> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 270bd4e512..6a2cc94abb 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -159,6 +159,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
>  	grep "sh\$" shell
>  '
>  
> +test_expect_success 'GIT_ATTR_SYSTEM points to the correct location' '

I found it somewhat funny to claim we are checking that the variable
points to the "correct" location, while the test is happy as long as
it points at any path.

> +	test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM &&
> +	(
> +		sane_unset GIT_ATTR_NOSYSTEM &&
> +		git var GIT_ATTR_SYSTEM >path &&
> +		test "$(cat path)" != ""
> +	)
> +'
> +
> +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
> +	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
> +	XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL >path &&
> +	test "$(cat path)" = "$TRASHDIR/.config/git/attributes" &&
> +	(
> +		sane_unset XDG_CONFIG_HOME &&
> +		HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL >path &&
> +		test "$(cat path)" = "$TRASHDIR/.config/git/attributes"
> +	)
> +'

This one is much less funny ;-)  Very nice.