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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 22, 2023, 21:17 UTC
Message-ID: <xmqqcz1ndwnz.fsf@gitster.g>
In-Reply-To: <20230622195059.320593-3-sandals@crustytoothpaste.net>

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

Show 17 quoted lines

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

Show 5 quoted lines

> -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()".

Show 24 quoted lines

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

Show 9 quoted lines

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

Show 18 quoted lines

> +	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.