[PATCH 0/3] Additional variables for git var

[PATCH 0/3] Additional variables for git var

[brian m. carlson]

On many Unix systems, we have a good idea where Git's configuration
files and the shell it uses are located.  However, there are some
systems where that's not the case, such as Windows and with Homebrew,
where the expected files might be found in another location.

Right now, programs who would like to access things like the system
gitattributes or config file have to guess where the distributor of Git
placed these files, and with runtime relocation, it's not even
guaranteed that these will be in a fixed place from invocation to
invocation.  As a result, we need a way to query Git about the location
of these files.

This series introduces five new configuration variables that refer to
the shell path, the system and global gitattributes files, and the
system and global config files.  The global values are not technically
needed, since they should be computable from the environment alone, but
they are added to make life easier for users.

The shell path is especially useful on Windows, where Git usually
provides the POSIX shell, and is designed to make using programs that
interact with Git or Unix-like environment variables (e.g., `EDITOR` and
`VISUAL`) easier.

The gitattributes files are primarily for the benefit of Git LFS, but of
course the goal is to be generally useful.

The curious reviewer may ask, "Why are these in `git var` and not `git
rev-parse`?"  The answer is that these refer to configuration (albeit
partially at compile time), and thus are like the existing GIT_EDITOR,
and unlike what `git rev-parse` produces, which is almost completely
limited to paths and metadata related specifically to a given
repository.

This should be a relatively straightforward series, but of course any
feedback is welcome.

brian m. carlson (3):
  var: add support for listing the shell
  var: add attributes files locations
  var: add config file locations

 Documentation/git-var.txt | 23 +++++++++
 attr.c                    |  6 +--
 attr.h                    |  4 ++
 builtin/var.c             | 99 +++++++++++++++++++++++++++++++++++----
 t/t0007-git-var.sh        | 98 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 219 insertions(+), 11 deletions(-)

[PATCH 1/3] var: add support for listing the shell

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

On most Unix systems, finding a suitable shell is easy: one simply uses
"sh" with an appropriate PATH value.  However, in many Windows
environments, the shell is shipped alongside Git, and it may or may not
be in PATH, even if Git is.

In such an environment, it can be very helpful to query Git for the
shell it's using, since other tools may want to use the same shell as
well.  To help them out, let's add a variable, GIT_SHELL_PATH, that
points to the location of the shell.

On Unix, we know our shell must be executable to be functional, so
assume that the distributor has correctly configured their environment,
and use that as a basic test.  On Git for Windows, we know that our
shell will be one of a few fixed values, all of which end in "sh" (such
as "bash").  This seems like it might be a nice test on Unix as well,
since it is customary for all shells to end in "sh", but there probably
exist such systems that don't have such a configuration, so be careful
here not to break them.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  3 +++
 builtin/var.c             |  6 ++++++
 t/t0007-git-var.sh        | 12 ++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f40202b8e3..f0f647e14a 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -71,6 +71,9 @@ endif::git-default-pager[]
 GIT_DEFAULT_BRANCH::
     The name of the first branch created in newly initialized repositories.
 
+GIT_SHELL_PATH::
+    The path of the binary providing the POSIX shell for commands which use the shell.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index 2149998980..f97178eed1 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -36,6 +36,11 @@ static const char *default_branch(int flag)
 	return git_default_branch_name(1);
 }
 
+static const char *shell_path(int flag)
+{
+	return SHELL_PATH;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -47,6 +52,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_SEQUENCE_EDITOR", sequence_editor },
 	{ "GIT_PAGER", pager },
 	{ "GIT_DEFAULT_BRANCH", default_branch },
+	{ "GIT_SHELL_PATH", shell_path },
 	{ "", NULL },
 };
 
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index eeb8539c1b..270bd4e512 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -147,6 +147,18 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment
 	)
 '
 
+test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
+	git var GIT_SHELL_PATH >shell &&
+	test -x "$(cat shell)"
+'
+
+# We know in this environment that our shell will be one of a few fixed values
+# that all end in "sh".
+test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
+	git var GIT_SHELL_PATH >shell &&
+	grep "sh\$" shell
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

Re: [PATCH 1/3] var: add support for listing the shell

[Eric Sunshine]

On Thu, Jun 22, 2023 at 4:03 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On most Unix systems, finding a suitable shell is easy: one simply uses
> "sh" with an appropriate PATH value.  However, in many Windows
> environments, the shell is shipped alongside Git, and it may or may not
> be in PATH, even if Git is.
>
> In such an environment, it can be very helpful to query Git for the
> shell it's using, since other tools may want to use the same shell as
> well.  To help them out, let's add a variable, GIT_SHELL_PATH, that
> points to the location of the shell.
>
> On Unix, we know our shell must be executable to be functional, so
> assume that the distributor has correctly configured their environment,
> and use that as a basic test.  On Git for Windows, we know that our
> shell will be one of a few fixed values, all of which end in "sh" (such
> as "bash").  This seems like it might be a nice test on Unix as well,
> since it is customary for all shells to end in "sh", but there probably
> exist such systems that don't have such a configuration, so be careful
> here not to break them.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> @@ -147,6 +147,18 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment
> +test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
> +       git var GIT_SHELL_PATH >shell &&
> +       test -x "$(cat shell)"
> +'

This can be implemented more simply without a temporary file:

    shpath=$(git var GIT_SHELL_PATH) &&
    test -x "$shpath"

This is safe since the exit code of the Git command is preserved
across the `shpath=...` assignment.

> +# We know in this environment that our shell will be one of a few fixed values
> +# that all end in "sh".
> +test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
> +       git var GIT_SHELL_PATH >shell &&
> +       grep "sh\$" shell
> +'

Similarly, there is no need for a temporary file or an extra process.
This can all be done entirely in the shell itself:

    shpath=$(git var GIT_SHELL_PATH) &&
    case "$shpath" in
    *sh) ;;
    *) return 1
    esac

Re: [PATCH 1/3] var: add support for listing the shell

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This can be implemented more simply without a temporary file:
>
>     shpath=$(git var GIT_SHELL_PATH) &&
>     test -x "$shpath"
>
> This is safe since the exit code of the Git command is preserved
> across the `shpath=...` assignment.

Correct.  I also suspect that we want to add test_path_is_executable
helper next to test_path_is_{file,dir,missing} helpers and list it
in t/README.  One downside of your approach is that the output from
the command is only in $shpath and cannot be observed easily in the
$TRASH_DIRECTORY after the test fails, but with such a helper we can
report the problematic path when the expectation fails.

Thanks.

Re: [PATCH 1/3] var: add support for listing the shell

[Eric Sunshine]

On Thu, Jun 22, 2023 at 5:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > This can be implemented more simply without a temporary file:
> >
> >     shpath=$(git var GIT_SHELL_PATH) &&
> >     test -x "$shpath"
> >
> > This is safe since the exit code of the Git command is preserved
> > across the `shpath=...` assignment.
>
> Correct.  I also suspect that we want to add test_path_is_executable
> helper next to test_path_is_{file,dir,missing} helpers and list it
> in t/README.  One downside of your approach is that the output from
> the command is only in $shpath and cannot be observed easily in the
> $TRASH_DIRECTORY after the test fails, but with such a helper we can
> report the problematic path when the expectation fails.

Good idea. A test_path_is_executable() function would also be a good
place to encapsulate the Windows-specific hackiness.

Re: [PATCH 1/3] var: add support for listing the shell

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On 2023-06-22 at 21:05:39, Junio C Hamano wrote:
> Correct.  I also suspect that we want to add test_path_is_executable
> helper next to test_path_is_{file,dir,missing} helpers and list it
> in t/README.  One downside of your approach is that the output from
> the command is only in $shpath and cannot be observed easily in the
> $TRASH_DIRECTORY after the test fails, but with such a helper we can
> report the problematic path when the expectation fails.

At first glance, I thought that was a good idea, too, but unfortunately
there is no way to make that work on Windows.  That's why all of our
tests skip those assertions with POSIXPERM, and why my tests
specifically look for something different on Windows.

We could in theory just make it always succeed there, but my concern
with writing such a function is that people will think it works
generally, when in fact it does not.  That's why, typically throughout
the codebase, we specifically use "test -x".
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/3] var: add support for listing the shell

[Junio C Hamano]

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

> On 2023-06-22 at 21:05:39, Junio C Hamano wrote:
>> Correct.  I also suspect that we want to add test_path_is_executable
>> helper next to test_path_is_{file,dir,missing} helpers and list it
>> in t/README.  One downside of your approach is that the output from
>> the command is only in $shpath and cannot be observed easily in the
>> $TRASH_DIRECTORY after the test fails, but with such a helper we can
>> report the problematic path when the expectation fails.
>
> At first glance, I thought that was a good idea, too, but unfortunately
> there is no way to make that work on Windows.  That's why all of our
> tests skip those assertions with POSIXPERM, and why my tests
> specifically look for something different on Windows.
>
> We could in theory just make it always succeed there, but my concern
> with writing such a function is that people will think it works
> generally, when in fact it does not.  That's why, typically throughout
> the codebase, we specifically use "test -x".

Hmph.  I would have thought that test_path_is_executable that is
based on "test -x" and gives a diagnosis when "test -x" fails would
be better than using bare "test -x" and be silent, even on Windows.

Re: [PATCH 1/3] var: add support for listing the shell

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On 2023-06-22 at 20:42:31, Eric Sunshine wrote:
> This can be implemented more simply without a temporary file:
> 
>     shpath=$(git var GIT_SHELL_PATH) &&
>     test -x "$shpath"
> 
> This is safe since the exit code of the Git command is preserved
> across the `shpath=...` assignment.

I can do this in v2.

> Similarly, there is no need for a temporary file or an extra process.
> This can all be done entirely in the shell itself:
> 
>     shpath=$(git var GIT_SHELL_PATH) &&
>     case "$shpath" in
>     *sh) ;;
>     *) return 1
>     esac

That's true, but this is much uglier and harder to read.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Currently, there are some programs which would like to read and parse
the gitattributes files at the global or system levels.  However, it's
not always obvious where these files live, especially for the system
file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

It's not reasonable to expect all callers of Git to intuitively know
where the Git distributor or user has configured these locations to
be, so add some entries to allow us to determine their location.  Honor
the GIT_ATTR_NOSYSTEM environment variable if one is specified.  Expose
the accessor functions in a way that we can reuse them from within the
var code.

In order to make our paths consistent on Windows and also use the same
form as paths use in "git rev-parse", let's normalize the path before we
return it.  This results in Windows-style paths that use slashes, which
is convenient for making our tests function in a consistent way across
platforms.  Note that this requires that some of our values be freed, so
let's add a flag about whether the value needs to be freed and use it
accordingly.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  6 ++++++
 attr.c                    |  6 +++---
 attr.h                    |  4 ++++
 builtin/var.c             | 45 ++++++++++++++++++++++++++++++++-------
 t/t0007-git-var.sh        | 20 +++++++++++++++++
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f0f647e14a..dfbe5cb52b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -74,6 +74,12 @@ GIT_DEFAULT_BRANCH::
 GIT_SHELL_PATH::
     The path of the binary providing the POSIX shell for commands which use the shell.
 
+GIT_ATTR_SYSTEM::
+    The path to the system linkgit:gitattributes[5] file, if one is enabled.
+
+GIT_ATTR_GLOBAL::
+    The path to the global (per-user) linkgit:gitattributes[5] file.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/attr.c b/attr.c
index d45d34058d..822d214053 100644
--- a/attr.c
+++ b/attr.c
@@ -870,7 +870,7 @@ static struct attr_stack *read_attr(struct index_state *istate,
 	return res;
 }
 
-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;
 }
 
-static int git_attr_system(void)
+int git_attr_system(void)
 {
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
diff --git a/attr.h b/attr.h
index 676bd17ce2..05a2e4c622 100644
--- a/attr.h
+++ b/attr.h
@@ -227,4 +227,8 @@ void git_attr_set_direction(enum git_attr_direction new_direction);
 
 void attr_start(void);
 
+const char *git_etc_gitattributes(void);
+const char *get_home_gitattributes(void);
+int git_attr_system(void);
+
 #endif /* ATTR_H */
diff --git a/builtin/var.c b/builtin/var.c
index f97178eed1..b9e2f23697 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -4,6 +4,7 @@
  * Copyright (C) Eric Biederman, 2005
  */
 #include "builtin.h"
+#include "attr.h"
 #include "config.h"
 #include "editor.h"
 #include "ident.h"
@@ -41,18 +42,41 @@ static const char *shell_path(int flag)
 	return SHELL_PATH;
 }
 
+static const char *git_attr_val_system(int flag)
+{
+	if (git_attr_system()) {
+		char *file = xstrdup(git_etc_gitattributes());
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static const char *git_attr_val_global(int flag)
+{
+	char *file = xstrdup(get_home_gitattributes());
+	if (file) {
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
 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 },
 };
 
@@ -62,8 +86,11 @@ static void list_vars(void)
 	const char *val;
 
 	for (ptr = git_vars; ptr->read; ptr++)
-		if ((val = ptr->read(0)))
+		if ((val = ptr->read(0))) {
 			printf("%s=%s\n", ptr->name, val);
+			if (ptr->free)
+				free((void *)val);
+		}
 }
 
 static const struct git_var *get_git_var(const char *var)
@@ -110,6 +137,8 @@ int cmd_var(int argc, const char **argv, const char *prefix UNUSED)
 		return 1;
 
 	printf("%s\n", val);
+	if (git_var->free)
+		free((void *)val);
 
 	return 0;
 }
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' '
+	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"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

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

[Derrick Stolee]

On 6/22/2023 3:50 PM, brian m. carlson wrote:

>  struct git_var {
>  	const char *name;
>  	const char *(*read)(int);
> +	int free;
>  };

I see you're expanding the git_var struct, and you do it
more in patch 3. At that point, there will be two
consecutive 'int' parameters, which can make things
unclear as to how to proceed.

>  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 },

This also makes for a big diff like this.

One way to solve for this is to use the more modern style
when initializing the structs:

static struct git_var git_vars[] = {
	{
		.name = "GIT_COMMITTER_IDENT",
		.read = git_author_info,
		.free = 0,
	},
	...
}

Bonus points if you do this conversion before modifying
the struct, as later changes will only add lines to the
existing initialization...

  static struct git_var git_vars[] = {
  	{
  		.name = "GIT_COMMITTER_IDENT",
  		.read = git_author_info,
+ 		.multivalued = 0,
  		.free = 0,
  	},
  	...
  }

Thanks,
-Stolee

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On 2023-06-22 at 20:19:46, Derrick Stolee wrote:
> One way to solve for this is to use the more modern style
> when initializing the structs:
> 
> static struct git_var git_vars[] = {
> 	{
> 		.name = "GIT_COMMITTER_IDENT",
> 		.read = git_author_info,
> 		.free = 0,
> 	},
> 	...
> }

Good idea, I've got this in for a v2.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Junio C Hamano]

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

> On 2023-06-22 at 20:19:46, Derrick Stolee wrote:
>> One way to solve for this is to use the more modern style
>> when initializing the structs:
>> 
>> static struct git_var git_vars[] = {
>> 	{
>> 		.name = "GIT_COMMITTER_IDENT",
>> 		.read = git_author_info,
>> 		.free = 0,
>> 	},
>> 	...
>> }
>
> Good idea, I've got this in for a v2.

Yeah, this would make it palatable ;-)

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

[Junio C Hamano]

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

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

[Eric Sunshine]

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Currently, there are some programs which would like to read and parse
> the gitattributes files at the global or system levels.  However, it's
> not always obvious where these files live, especially for the system
> file, which may have been hard-coded at compile time or computed
> dynamically based on the runtime prefix.
>
> It's not reasonable to expect all callers of Git to intuitively know
> where the Git distributor or user has configured these locations to
> be, so add some entries to allow us to determine their location.  Honor
> the GIT_ATTR_NOSYSTEM environment variable if one is specified.  Expose
> the accessor functions in a way that we can reuse them from within the
> var code.
>
> In order to make our paths consistent on Windows and also use the same
> form as paths use in "git rev-parse", let's normalize the path before we
> return it.  This results in Windows-style paths that use slashes, which
> is convenient for making our tests function in a consistent way across
> platforms.  Note that this requires that some of our values be freed, so
> let's add a flag about whether the value needs to be freed and use it
> accordingly.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git 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' '
> +test_expect_success 'GIT_ATTR_SYSTEM points to the correct location' '
> +       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)" != ""
> +       )
> +'

Same observation as in patch [1/3]: no need for a temporary file:

    p=$(git var GIT_ATTR_SYSTEM) &&
    test -n "$p"

> +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
> +       TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&

The reference to $(pwd) is unnecessary, thus potentially confusing. Simpler:

    TRASHDIR="$(test-tool path-utils normalize_path_copy .)" &&

> +       XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL >path &&
> +       test "$(cat path)" = "$TRASHDIR/.config/git/attributes" &&

Same observation about unnecessary temporary file:

    p=$(XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL) &&
    test "$p" = "$TRASHDIR/.config/git/attributes" &&

> +       (
> +               sane_unset XDG_CONFIG_HOME &&
> +               HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL >path &&
> +               test "$(cat path)" = "$TRASHDIR/.config/git/attributes"
> +       )

And here too.

> +'

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On 2023-06-22 at 21:18:46, Eric Sunshine wrote:
> The reference to $(pwd) is unnecessary, thus potentially confusing. Simpler:
> 
>     TRASHDIR="$(test-tool path-utils normalize_path_copy .)" &&

Maybe.  I spent a lot of time fighting with Windows and its path
handling here (as much as writing the initial series).  If that's
possible and produces an acceptable result there, I can consider it for
v2.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Eric Sunshine]

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Currently, there are some programs which would like to read and parse
> the gitattributes files at the global or system levels.  However, it's
> not always obvious where these files live, especially for the system
> file, which may have been hard-coded at compile time or computed
> dynamically based on the runtime prefix.
>
> It's not reasonable to expect all callers of Git to intuitively know
> where the Git distributor or user has configured these locations to
> be, so add some entries to allow us to determine their location.  Honor
> the GIT_ATTR_NOSYSTEM environment variable if one is specified.  Expose
> the accessor functions in a way that we can reuse them from within the
> var code.
>
> In order to make our paths consistent on Windows and also use the same
> form as paths use in "git rev-parse", let's normalize the path before we
> return it.  This results in Windows-style paths that use slashes, which
> is convenient for making our tests function in a consistent way across
> platforms.  Note that this requires that some of our values be freed, so
> let's add a flag about whether the value needs to be freed and use it
> accordingly.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git a/attr.h b/attr.h
> @@ -227,4 +227,8 @@ void git_attr_set_direction(enum git_attr_direction new_direction);
> +const char *git_etc_gitattributes(void);
> +const char *get_home_gitattributes(void);
> +int git_attr_system(void);

I forgot to mention that it would be appreciated if you document these
new public declarations. Even a one-sentence comment would be helpful.
For instance:

    /* Return the location of the personal .gitattributes file. */
    const char *get_home_gitattributes(void);

[PATCH 3/3] var: add config file locations

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Much like with attributes files, sometimes programs would like to know
the location of configuration files at the global or system levels.
However, it isn't always clear where these may live, especially for the
system file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

Since other parties cannot intuitively know how Git was compiled and
where it looks for these files, help them by providing variables that
can be queried.  Because we have multiple paths for global config
values, print them in order from highest to lowest priority, and be sure
to split on newlines so that "git var -l" produces two entries for the
global value.

However, be careful not to split all values on newlines, since our
editor values could well contain such characters, and we don't want to
split them in such a case.

Note in the documentation that some values may contain multiple paths
and that callers should be prepared for that fact.  This helps people
write code that will continue to work in the event we allow multiple
items elsewhere in the future.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt | 14 ++++++++
 builtin/var.c             | 68 +++++++++++++++++++++++++++++++++------
 t/t0007-git-var.sh        | 66 +++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index dfbe5cb52b..c38fb3968b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -80,6 +80,20 @@ GIT_ATTR_SYSTEM::
 GIT_ATTR_GLOBAL::
     The path to the global (per-user) linkgit:gitattributes[5] file.
 
+GIT_CONFIG_SYSTEM::
+    The path to the system configuration file, if one is enabled.
+
+GIT_CONFIG_GLOBAL::
+    The path to the global (per-user) configuration files, if any.
+
+Most path values contain only one value. However, some can contain multiple
+values, which are separated by newlines, and are listed in order from highest to
+lowest priority.  Callers should be prepared for any such path value to contain
+multiple items.
+
+Note that paths are printed even if they do not exist, but not if they are
+disabled by other environment variables.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index b9e2f23697..8a47b74777 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -62,21 +62,59 @@ static const char *git_attr_val_global(int flag)
 	return NULL;
 }
 
+static const char *git_config_val_system(int flag)
+{
+	if (git_config_system()) {
+		char *file = git_system_config();
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static const char *git_config_val_global(int flag)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *user, *xdg;
+	size_t unused;
+
+	git_global_config(&user, &xdg);
+	if (xdg && *xdg) {
+		normalize_path_copy(xdg, xdg);
+		strbuf_addf(&buf, "%s\n", xdg);
+	}
+	if (user && *user) {
+		normalize_path_copy(user, user);
+		strbuf_addf(&buf, "%s\n", user);
+	}
+	free(xdg);
+	free(user);
+	strbuf_trim_trailing_newline(&buf);
+	if (buf.len == 0) {
+		strbuf_release(&buf);
+		return NULL;
+	}
+	return strbuf_detach(&buf, &unused);
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
+	int multivalued;
 	int free;
 };
 static struct git_var git_vars[] = {
-	{ "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 },
+	{ "GIT_COMMITTER_IDENT", git_committer_info, 0, 0 },
+	{ "GIT_AUTHOR_IDENT",   git_author_info, 0, 0 },
+	{ "GIT_EDITOR", editor, 0, 0 },
+	{ "GIT_SEQUENCE_EDITOR", sequence_editor, 0, 0 },
+	{ "GIT_PAGER", pager, 0, 0 },
+	{ "GIT_DEFAULT_BRANCH", default_branch, 0, 9 },
+	{ "GIT_SHELL_PATH", shell_path, 0, 0 },
+	{ "GIT_ATTR_SYSTEM", git_attr_val_system, 0, 1 },
+	{ "GIT_ATTR_GLOBAL", git_attr_val_global, 0, 1 },
+	{ "GIT_CONFIG_SYSTEM", git_config_val_system, 0, 1 },
+	{ "GIT_CONFIG_GLOBAL", git_config_val_global, 1, 1 },
 	{ "", NULL },
 };
 
@@ -87,7 +125,17 @@ static void list_vars(void)
 
 	for (ptr = git_vars; ptr->read; ptr++)
 		if ((val = ptr->read(0))) {
-			printf("%s=%s\n", ptr->name, val);
+			if (ptr->multivalued && *val) {
+				struct string_list list = STRING_LIST_INIT_DUP;
+				int i;
+
+				string_list_split(&list, val, '\n', -1);
+				for (i = 0; i < list.nr; i++)
+					printf("%s=%s\n", ptr->name, list.items[i].string);
+				string_list_clear(&list, 0);
+			} else {
+				printf("%s=%s\n", ptr->name, val);
+			}
 			if (ptr->free)
 				free((void *)val);
 		}
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 6a2cc94abb..d519c2f441 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -179,6 +179,49 @@ test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
 	)
 '
 
+test_expect_success 'GIT_CONFIG_SYSTEM points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	test_must_fail env GIT_CONFIG_NOSYSTEM=1 git var GIT_CONFIG_SYSTEM &&
+	(
+		sane_unset GIT_CONFIG_NOSYSTEM &&
+		git var GIT_CONFIG_SYSTEM >path &&
+		test "$(cat path)" != "" &&
+		GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM >path &&
+		if test_have_prereq MINGW
+		then
+			test "$(cat path)" = "nul"
+		else
+			test "$(cat path)" = "/dev/null"
+		fi &&
+		GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM >path &&
+		test "$(cat path)" = "$TRASHDIR/gitconfig"
+	)
+'
+
+test_expect_success 'GIT_CONFIG_GLOBAL points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var GIT_CONFIG_GLOBAL >actual &&
+	echo "$TRASHDIR/foo/git/config" >expected &&
+	echo "$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected actual &&
+	(
+		sane_unset XDG_CONFIG_HOME &&
+		HOME="$TRASHDIR" git var GIT_CONFIG_GLOBAL >actual &&
+		echo "$TRASHDIR/.config/git/config" >expected &&
+		echo "$TRASHDIR/.gitconfig" >>expected &&
+		test_cmp expected actual &&
+		GIT_CONFIG_GLOBAL=/dev/null git var GIT_CONFIG_GLOBAL >path &&
+		if test_have_prereq MINGW
+		then
+			test "$(cat path)" = "nul"
+		else
+			test "$(cat path)" = "/dev/null"
+		fi &&
+		GIT_CONFIG_GLOBAL="$TRASHDIR/gitconfig" git var GIT_CONFIG_GLOBAL >path &&
+		test "$(cat path)" = "$TRASHDIR/gitconfig"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.
@@ -196,6 +239,29 @@ test_expect_success 'git var -l lists config' '
 	test_cmp expect actual.bare
 '
 
+test_expect_success 'git var -l lists multiple global configs' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var -l >actual &&
+	grep "^GIT_CONFIG_GLOBAL=" actual >filtered &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/foo/git/config" >expected &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected filtered
+'
+
+test_expect_success 'git var -l does not split multiline editors' '
+	(
+		GIT_EDITOR="!f() {
+			echo Hello!
+		}; f" &&
+		export GIT_EDITOR &&
+		echo "GIT_EDITOR=$GIT_EDITOR" >expected &&
+		git var -l >var &&
+		cat var &&
+		sed -n -e "/^GIT_EDITOR/,\$p" var | head -n 3 >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'listing and asking for variables are exclusive' '
 	test_must_fail git var -l GIT_COMMITTER_IDENT
 '

Re: [PATCH 3/3] var: add config file locations

[Eric Sunshine]

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Much like with attributes files, sometimes programs would like to know
> the location of configuration files at the global or system levels.
> However, it isn't always clear where these may live, especially for the
> system file, which may have been hard-coded at compile time or computed
> dynamically based on the runtime prefix.
>
> Since other parties cannot intuitively know how Git was compiled and
> where it looks for these files, help them by providing variables that
> can be queried.  Because we have multiple paths for global config
> values, print them in order from highest to lowest priority, and be sure
> to split on newlines so that "git var -l" produces two entries for the
> global value.
>
> However, be careful not to split all values on newlines, since our
> editor values could well contain such characters, and we don't want to
> split them in such a case.
>
> Note in the documentation that some values may contain multiple paths
> and that callers should be prepared for that fact.  This helps people
> write code that will continue to work in the event we allow multiple
> items elsewhere in the future.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git a/builtin/var.c b/builtin/var.c
> @@ -62,21 +62,59 @@ static const char *git_attr_val_global(int flag)
>  struct git_var {
>         const char *name;
>         const char *(*read)(int);
> +       int multivalued;
>         int free;
>  };
>  static struct git_var git_vars[] = {
> +       { "GIT_COMMITTER_IDENT", git_committer_info, 0, 0 },
> +       { "GIT_AUTHOR_IDENT",   git_author_info, 0, 0 },
> +       { "GIT_EDITOR", editor, 0, 0 },
> +       { "GIT_SEQUENCE_EDITOR", sequence_editor, 0, 0 },
> +       { "GIT_PAGER", pager, 0, 0 },
> +       { "GIT_DEFAULT_BRANCH", default_branch, 0, 9 },

Why "9"?

> +       { "GIT_SHELL_PATH", shell_path, 0, 0 },
> +       { "GIT_ATTR_SYSTEM", git_attr_val_system, 0, 1 },
> +       { "GIT_ATTR_GLOBAL", git_attr_val_global, 0, 1 },
> +       { "GIT_CONFIG_SYSTEM", git_config_val_system, 0, 1 },
> +       { "GIT_CONFIG_GLOBAL", git_config_val_global, 1, 1 },
>         { "", NULL },
>  };
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> @@ -179,6 +179,49 @@ test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
> +test_expect_success 'GIT_CONFIG_SYSTEM points to the correct location' '
> +       TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&

Same comment as in [2/3]: $(pwd) is unnecessary. Simpler:

    TRASHDIR="$(test-tool path-utils normalize_path_copy .)" &&

> +       test_must_fail env GIT_CONFIG_NOSYSTEM=1 git var GIT_CONFIG_SYSTEM &&
> +       (
> +               sane_unset GIT_CONFIG_NOSYSTEM &&
> +               git var GIT_CONFIG_SYSTEM >path &&
> +               test "$(cat path)" != "" &&
> +               GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM >path &&
> +               if test_have_prereq MINGW
> +               then
> +                       test "$(cat path)" = "nul"
> +               else
> +                       test "$(cat path)" = "/dev/null"
> +               fi &&
> +               GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM >path &&
> +               test "$(cat path)" = "$TRASHDIR/gitconfig"
> +       )
> +'

Ditto regarding unnecessary temporary file.

> +test_expect_success 'GIT_CONFIG_GLOBAL points to the correct location' '
> +       TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&

Ditto regarding $(pwd).

> @@ -196,6 +239,29 @@ test_expect_success 'git var -l lists config' '
> +test_expect_success 'git var -l does not split multiline editors' '
> +       (
> +               GIT_EDITOR="!f() {
> +                       echo Hello!
> +               }; f" &&
> +               export GIT_EDITOR &&
> +               echo "GIT_EDITOR=$GIT_EDITOR" >expected &&
> +               git var -l >var &&
> +               cat var &&

Is this `cat` leftover debugging code?

> +               sed -n -e "/^GIT_EDITOR/,\$p" var | head -n 3 >actual &&
> +               test_cmp expected actual
> +       )
> +'

[PATCH v2 0/7] Additional variables for git var

[brian m. carlson]

On many Unix systems, we have a good idea where Git's configuration
files and the shell it uses are located.  However, there are some
systems where that's not the case, such as Windows and with Homebrew,
where the expected files might be found in another location.

Right now, programs who would like to access things like the system
gitattributes or config file have to guess where the distributor of Git
placed these files, and with runtime relocation, it's not even
guaranteed that these will be in a fixed place from invocation to
invocation.  As a result, we need a way to query Git about the location
of these files.

This series introduces five new configuration variables that refer to
the shell path, the system and global gitattributes files, and the
system and global config files.  The global values are not technically
needed, since they should be computable from the environment alone, but
they are added to make life easier for users.

I have adopted most of the changes suggested, but I have specifically
not adopted the use of `.` in our tests instead of `$(pwd)`.  That
breaks the tests on Windows, the functionality of which has already been
the major portion of the time spent on this series despite careful
forethought about functionality there, and as a non-Windows user, I'm
simply not capable of or interested in futzing around with it more.
Parties who are unhappy with that are invited to send a follow-up patch
after this series lands.

Changes since v1:
* Format variables with C99 initializers.
* Minimize use of temporary files in the tests.
* Remove debugging statement.
* Avoid grep where possible in tests.
* Duplicate memory rather than optionally choosing whether to free data.
* Add and document test_file_is_executable.
* Possibly more things which I have forgotten.

brian m. carlson (7):
  t: add a function to check executable bit
  var: add support for listing the shell
  var: format variable structure with C99 initializers
  var: adjust memory allocation for strings
  attr: expose and rename accessor functions
  var: add attributes files locations
  var: add config file locations

 Documentation/git-var.txt |  23 +++++
 attr.c                    |  14 +--
 attr.h                    |   9 ++
 builtin/var.c             | 178 +++++++++++++++++++++++++++++++++-----
 t/README                  |   6 ++
 t/t0007-git-var.sh        | 100 +++++++++++++++++++++
 t/test-lib-functions.sh   |   9 ++
 7 files changed, 312 insertions(+), 27 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  20b7b85e98 t: add a function to check executable bit
1:  441dc45a86 ! 2:  7d92b4155f var: add support for listing the shell
    @@ t/t0007-git-var.sh: test_expect_success 'get GIT_SEQUENCE_EDITOR with configurat
      '
      
     +test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
    -+	git var GIT_SHELL_PATH >shell &&
    -+	test -x "$(cat shell)"
    ++	shellpath=$(git var GIT_SHELL_PATH) &&
    ++	test_path_is_executable "$shellpath"
     +'
     +
     +# We know in this environment that our shell will be one of a few fixed values
     +# that all end in "sh".
     +test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
    -+	git var GIT_SHELL_PATH >shell &&
    -+	grep "sh\$" shell
    ++	shellpath=$(git var GIT_SHELL_PATH) &&
    ++	case "$shellpath" in
    ++	*sh) ;;
    ++	*) return 1;;
    ++	esac
     +'
     +
      # For git var -l, we check only a representative variable;
2:  3b6935f226 < -:  ---------- var: add attributes files locations
-:  ---------- > 3:  29c338d59c var: format variable structure with C99 initializers
-:  ---------- > 4:  2a2a762c44 var: adjust memory allocation for strings
-:  ---------- > 5:  c0c5c59df9 attr: expose and rename accessor functions
-:  ---------- > 6:  49a04bd142 var: add attributes files locations
3:  6a2b5494dd ! 7:  a8b4d9396b var: add config file locations
    @@ Documentation/git-var.txt: GIT_ATTR_SYSTEM::
      linkgit:git-commit-tree[1]
     
      ## builtin/var.c ##
    -@@ builtin/var.c: static const char *git_attr_val_global(int flag)
    +@@ builtin/var.c: static char *git_attr_val_global(int flag)
      	return NULL;
      }
      
    -+static const char *git_config_val_system(int flag)
    ++static char *git_config_val_system(int flag)
     +{
     +	if (git_config_system()) {
     +		char *file = git_system_config();
    @@ builtin/var.c: static const char *git_attr_val_global(int flag)
     +	return NULL;
     +}
     +
    -+static const char *git_config_val_global(int flag)
    ++static char *git_config_val_global(int flag)
     +{
     +	struct strbuf buf = STRBUF_INIT;
     +	char *user, *xdg;
    @@ builtin/var.c: static const char *git_attr_val_global(int flag)
     +
      struct git_var {
      	const char *name;
    - 	const char *(*read)(int);
    + 	char *(*read)(int);
     +	int multivalued;
    - 	int free;
      };
      static struct git_var git_vars[] = {
    --	{ "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 },
    -+	{ "GIT_COMMITTER_IDENT", git_committer_info, 0, 0 },
    -+	{ "GIT_AUTHOR_IDENT",   git_author_info, 0, 0 },
    -+	{ "GIT_EDITOR", editor, 0, 0 },
    -+	{ "GIT_SEQUENCE_EDITOR", sequence_editor, 0, 0 },
    -+	{ "GIT_PAGER", pager, 0, 0 },
    -+	{ "GIT_DEFAULT_BRANCH", default_branch, 0, 9 },
    -+	{ "GIT_SHELL_PATH", shell_path, 0, 0 },
    -+	{ "GIT_ATTR_SYSTEM", git_attr_val_system, 0, 1 },
    -+	{ "GIT_ATTR_GLOBAL", git_attr_val_global, 0, 1 },
    -+	{ "GIT_CONFIG_SYSTEM", git_config_val_system, 0, 1 },
    -+	{ "GIT_CONFIG_GLOBAL", git_config_val_global, 1, 1 },
    - 	{ "", NULL },
    + 	{
    + 		.name = "GIT_COMMITTER_IDENT",
    + 		.read = committer,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_AUTHOR_IDENT",
    + 		.read = author,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_EDITOR",
    + 		.read = editor,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_SEQUENCE_EDITOR",
    + 		.read = sequence_editor,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_PAGER",
    + 		.read = pager,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_DEFAULT_BRANCH",
    + 		.read = default_branch,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_SHELL_PATH",
    + 		.read = shell_path,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_ATTR_SYSTEM",
    + 		.read = git_attr_val_system,
    ++		.multivalued = 0,
    + 	},
    + 	{
    + 		.name = "GIT_ATTR_GLOBAL",
    + 		.read = git_attr_val_global,
    ++		.multivalued = 0,
    ++	},
    ++	{
    ++		.name = "GIT_CONFIG_SYSTEM",
    ++		.read = git_config_val_system,
    ++		.multivalued = 0,
    ++	},
    ++	{
    ++		.name = "GIT_CONFIG_GLOBAL",
    ++		.read = git_config_val_global,
    ++		.multivalued = 1,
    + 	},
    + 	{
    + 		.name = "",
    + 		.read = NULL,
    ++		.multivalued = 0,
    + 	},
      };
      
     @@ builtin/var.c: static void list_vars(void)
    @@ builtin/var.c: static void list_vars(void)
     +			} else {
     +				printf("%s=%s\n", ptr->name, val);
     +			}
    - 			if (ptr->free)
    - 				free((void *)val);
    + 			free(val);
      		}
    + }
     
      ## t/t0007-git-var.sh ##
     @@ t/t0007-git-var.sh: test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
    @@ t/t0007-git-var.sh: test_expect_success 'GIT_ATTR_GLOBAL points to the correct l
     +	test_must_fail env GIT_CONFIG_NOSYSTEM=1 git var GIT_CONFIG_SYSTEM &&
     +	(
     +		sane_unset GIT_CONFIG_NOSYSTEM &&
    -+		git var GIT_CONFIG_SYSTEM >path &&
    -+		test "$(cat path)" != "" &&
    -+		GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM >path &&
    ++		systempath=$(git var GIT_CONFIG_SYSTEM) &&
    ++		test "$systempath" != "" &&
    ++		systempath=$(GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM) &&
     +		if test_have_prereq MINGW
     +		then
    -+			test "$(cat path)" = "nul"
    ++			test "$systempath" = "nul"
     +		else
    -+			test "$(cat path)" = "/dev/null"
    ++			test "$systempath" = "/dev/null"
     +		fi &&
    -+		GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM >path &&
    -+		test "$(cat path)" = "$TRASHDIR/gitconfig"
    ++		systempath=$(GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM) &&
    ++		test "$systempath" = "$TRASHDIR/gitconfig"
     +	)
     +'
     +
    @@ t/t0007-git-var.sh: test_expect_success 'GIT_ATTR_GLOBAL points to the correct l
     +		echo "$TRASHDIR/.config/git/config" >expected &&
     +		echo "$TRASHDIR/.gitconfig" >>expected &&
     +		test_cmp expected actual &&
    -+		GIT_CONFIG_GLOBAL=/dev/null git var GIT_CONFIG_GLOBAL >path &&
    ++		globalpath=$(GIT_CONFIG_GLOBAL=/dev/null git var GIT_CONFIG_GLOBAL) &&
     +		if test_have_prereq MINGW
     +		then
    -+			test "$(cat path)" = "nul"
    ++			test "$globalpath" = "nul"
     +		else
    -+			test "$(cat path)" = "/dev/null"
    ++			test "$globalpath" = "/dev/null"
     +		fi &&
    -+		GIT_CONFIG_GLOBAL="$TRASHDIR/gitconfig" git var GIT_CONFIG_GLOBAL >path &&
    -+		test "$(cat path)" = "$TRASHDIR/gitconfig"
    ++		globalpath=$(GIT_CONFIG_GLOBAL="$TRASHDIR/gitconfig" git var GIT_CONFIG_GLOBAL) &&
    ++		test "$globalpath" = "$TRASHDIR/gitconfig"
     +	)
     +'
     +
    @@ t/t0007-git-var.sh: test_expect_success 'git var -l lists config' '
     +		export GIT_EDITOR &&
     +		echo "GIT_EDITOR=$GIT_EDITOR" >expected &&
     +		git var -l >var &&
    -+		cat var &&
     +		sed -n -e "/^GIT_EDITOR/,\$p" var | head -n 3 >actual &&
     +		test_cmp expected actual
     +	)

[PATCH v2 1/7] t: add a function to check executable bit

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

In line with our other helper functions for paths, let's add a function
to check whether a path is executable, and if not, print a suitable
error message.  Document this function, and note that it must only be
used under the POSIXPERM prerequisite, since it doesn't otherwise work
on Windows.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 t/README                | 6 ++++++
 t/test-lib-functions.sh | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/t/README b/t/README
index bdfac4cceb..3e155011de 100644
--- a/t/README
+++ b/t/README
@@ -1098,6 +1098,12 @@ see test-lib-functions.sh for the full list and their options.
    the symbolic link in the file system and a part that does; then only
    the latter part need be protected by a SYMLINKS prerequisite (see below).
 
+ - test_path_is_executable
+
+   This tests whether a file is executable and prints an error message
+   if not. This must be used only under the POSIXPERM prerequisite
+   (see below).
+
  - test_oid_init
 
    This function loads facts and useful object IDs related to the hash
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b3864e22e9..2fa716c567 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -910,6 +910,15 @@ test_path_is_symlink () {
 	fi
 }
 
+test_path_is_executable () {
+	test "$#" -ne 1 && BUG "1 param"
+	if ! test -x "$1"
+	then
+		echo "$1 is not executable"
+		false
+	fi
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
 	test "$#" -ne 1 && BUG "1 param"

[PATCH v2 2/7] var: add support for listing the shell

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

On most Unix systems, finding a suitable shell is easy: one simply uses
"sh" with an appropriate PATH value.  However, in many Windows
environments, the shell is shipped alongside Git, and it may or may not
be in PATH, even if Git is.

In such an environment, it can be very helpful to query Git for the
shell it's using, since other tools may want to use the same shell as
well.  To help them out, let's add a variable, GIT_SHELL_PATH, that
points to the location of the shell.

On Unix, we know our shell must be executable to be functional, so
assume that the distributor has correctly configured their environment,
and use that as a basic test.  On Git for Windows, we know that our
shell will be one of a few fixed values, all of which end in "sh" (such
as "bash").  This seems like it might be a nice test on Unix as well,
since it is customary for all shells to end in "sh", but there probably
exist such systems that don't have such a configuration, so be careful
here not to break them.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  3 +++
 builtin/var.c             |  6 ++++++
 t/t0007-git-var.sh        | 15 +++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f40202b8e3..f0f647e14a 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -71,6 +71,9 @@ endif::git-default-pager[]
 GIT_DEFAULT_BRANCH::
     The name of the first branch created in newly initialized repositories.
 
+GIT_SHELL_PATH::
+    The path of the binary providing the POSIX shell for commands which use the shell.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index 2149998980..f97178eed1 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -36,6 +36,11 @@ static const char *default_branch(int flag)
 	return git_default_branch_name(1);
 }
 
+static const char *shell_path(int flag)
+{
+	return SHELL_PATH;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -47,6 +52,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_SEQUENCE_EDITOR", sequence_editor },
 	{ "GIT_PAGER", pager },
 	{ "GIT_DEFAULT_BRANCH", default_branch },
+	{ "GIT_SHELL_PATH", shell_path },
 	{ "", NULL },
 };
 
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index eeb8539c1b..e35f07afcb 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -147,6 +147,21 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment
 	)
 '
 
+test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
+	shellpath=$(git var GIT_SHELL_PATH) &&
+	test_path_is_executable "$shellpath"
+'
+
+# We know in this environment that our shell will be one of a few fixed values
+# that all end in "sh".
+test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
+	shellpath=$(git var GIT_SHELL_PATH) &&
+	case "$shellpath" in
+	*sh) ;;
+	*) return 1;;
+	esac
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

[PATCH v2 3/7] var: format variable structure with C99 initializers

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, we have only two items in our variable struct.  However, in
the future, we're going to add two more items.  To help keep our diffs
nice and tidy and make this structure easier to read, switch to use
C99-style initializers for our data.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/var.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index f97178eed1..bd2750b1bf 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -46,14 +46,38 @@ struct git_var {
 	const char *(*read)(int);
 };
 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 },
-	{ "", NULL },
+	{
+		.name = "GIT_COMMITTER_IDENT",
+		.read = git_committer_info,
+	},
+	{
+		.name = "GIT_AUTHOR_IDENT",
+		.read = git_author_info,
+	},
+	{
+		.name = "GIT_EDITOR",
+		.read = editor,
+	},
+	{
+		.name = "GIT_SEQUENCE_EDITOR",
+		.read = sequence_editor,
+	},
+	{
+		.name = "GIT_PAGER",
+		.read = pager,
+	},
+	{
+		.name = "GIT_DEFAULT_BRANCH",
+		.read = default_branch,
+	},
+	{
+		.name = "GIT_SHELL_PATH",
+		.read = shell_path,
+	},
+	{
+		.name = "",
+		.read = NULL,
+	},
 };
 
 static void list_vars(void)

[PATCH v2 4/7] var: adjust memory allocation for strings

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, all of our values are constants whose allocation is managed
elsewhere.  However, in the future, we'll have some variables whose
memory we will need to free.  To keep things consistent, let's make each
of our functions allocate its own memory and make the caller responsible
for freeing it.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/var.c | 45 +++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index bd2750b1bf..ce6a2231ca 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,47 +12,57 @@
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
-static const char *editor(int flag)
+static char *committer(int flag)
 {
-	return git_editor();
+	return xstrdup_or_null(git_committer_info(flag));
 }
 
-static const char *sequence_editor(int flag)
+static char *author(int flag)
 {
-	return git_sequence_editor();
+	return xstrdup_or_null(git_author_info(flag));
 }
 
-static const char *pager(int flag)
+static char *editor(int flag)
+{
+	return xstrdup_or_null(git_editor());
+}
+
+static char *sequence_editor(int flag)
+{
+	return xstrdup_or_null(git_sequence_editor());
+}
+
+static char *pager(int flag)
 {
 	const char *pgm = git_pager(1);
 
 	if (!pgm)
 		pgm = "cat";
-	return pgm;
+	return xstrdup(pgm);
 }
 
-static const char *default_branch(int flag)
+static char *default_branch(int flag)
 {
-	return git_default_branch_name(1);
+	return xstrdup_or_null(git_default_branch_name(1));
 }
 
-static const char *shell_path(int flag)
+static char *shell_path(int flag)
 {
-	return SHELL_PATH;
+	return xstrdup(SHELL_PATH);
 }
 
 struct git_var {
 	const char *name;
-	const char *(*read)(int);
+	char *(*read)(int);
 };
 static struct git_var git_vars[] = {
 	{
 		.name = "GIT_COMMITTER_IDENT",
-		.read = git_committer_info,
+		.read = committer,
 	},
 	{
 		.name = "GIT_AUTHOR_IDENT",
-		.read = git_author_info,
+		.read = author,
 	},
 	{
 		.name = "GIT_EDITOR",
@@ -83,11 +93,13 @@ static struct git_var git_vars[] = {
 static void list_vars(void)
 {
 	struct git_var *ptr;
-	const char *val;
+	char *val;
 
 	for (ptr = git_vars; ptr->read; ptr++)
-		if ((val = ptr->read(0)))
+		if ((val = ptr->read(0))) {
 			printf("%s=%s\n", ptr->name, val);
+			free(val);
+		}
 }
 
 static const struct git_var *get_git_var(const char *var)
@@ -113,7 +125,7 @@ static int show_config(const char *var, const char *value, void *cb)
 int cmd_var(int argc, const char **argv, const char *prefix UNUSED)
 {
 	const struct git_var *git_var;
-	const char *val;
+	char *val;
 
 	if (argc != 2)
 		usage(var_usage);
@@ -134,6 +146,7 @@ int cmd_var(int argc, const char **argv, const char *prefix UNUSED)
 		return 1;
 
 	printf("%s\n", val);
+	free(val);
 
 	return 0;
 }

Re: [PATCH v2 4/7] var: adjust memory allocation for strings

[Junio C Hamano]

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

> From: "brian m. carlson" <bk2204@github.com>
>
> Right now, all of our values are constants whose allocation is managed
> elsewhere.  However, in the future, we'll have some variables whose
> memory we will need to free.  To keep things consistent, let's make each
> of our functions allocate its own memory and make the caller responsible
> for freeing it.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
>  builtin/var.c | 45 +++++++++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 16 deletions(-)

Making everybody allocate like this patch does is also fine, but
FWIW, with [3/7], we can selectively set ".need_free = 1" for
minority of elements in the array that returns an allocated piece of
memory without making the source code too noisy (as the array
elements for existing majority can omit ".need_free = 0" and leave
it to the default initialization), so I am not opposed to the "we
set .need_free bit only for those that we allocate" approach taken
with the previous round.

Thanks.

[PATCH v2 5/7] attr: expose and rename accessor functions

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, the functions which determine the current system and global
gitattributes files are not exposed.  We'd like to use them in a future
commit, but they're not ideally named.  Rename them to something more
suitable as a public interface, expose them, and document them.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 attr.c | 14 +++++++-------
 attr.h |  9 +++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/attr.c b/attr.c
index d45d34058d..66203ce322 100644
--- a/attr.c
+++ b/attr.c
@@ -870,7 +870,7 @@ static struct attr_stack *read_attr(struct index_state *istate,
 	return res;
 }
 
-static const char *git_etc_gitattributes(void)
+const char *git_attr_system_file(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 *git_attr_global_file(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;
 }
 
-static int git_attr_system(void)
+int git_attr_system_is_enabled(void)
 {
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
@@ -920,14 +920,14 @@ static void bootstrap_attr_stack(struct index_state *istate,
 	push_stack(stack, e, NULL, 0);
 
 	/* system-wide frame */
-	if (git_attr_system()) {
-		e = read_attr_from_file(git_etc_gitattributes(), flags);
+	if (git_attr_system_is_enabled()) {
+		e = read_attr_from_file(git_attr_system_file(), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
 	/* home directory */
-	if (get_home_gitattributes()) {
-		e = read_attr_from_file(get_home_gitattributes(), flags);
+	if (git_attr_global_file()) {
+		e = read_attr_from_file(git_attr_global_file(), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
diff --git a/attr.h b/attr.h
index 676bd17ce2..2b745df405 100644
--- a/attr.h
+++ b/attr.h
@@ -227,4 +227,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction);
 
 void attr_start(void);
 
+/* Return the system gitattributes file. */
+const char *git_attr_system_file(void);
+
+/* Return the global gitattributes file, if any. */
+const char *git_attr_global_file(void);
+
+/* Return whether the system gitattributes file is enabled and should be used. */
+int git_attr_system_is_enabled(void);
+
 #endif /* ATTR_H */

Re: [PATCH v2 5/7] attr: expose and rename accessor functions

[Junio C Hamano]

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

> From: "brian m. carlson" <bk2204@github.com>
>
> Right now, the functions which determine the current system and global
> gitattributes files are not exposed.  We'd like to use them in a future
> commit, but they're not ideally named.  Rename them to something more
> suitable as a public interface, expose them, and document them.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
>  attr.c | 14 +++++++-------
>  attr.h |  9 +++++++++
>  2 files changed, 16 insertions(+), 7 deletions(-)

I very much like the updated names.  Thanks for a clean-up that is
long overdue for about a dozen years or so.

[PATCH v2 6/7] var: add attributes files locations

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Currently, there are some programs which would like to read and parse
the gitattributes files at the global or system levels.  However, it's
not always obvious where these files live, especially for the system
file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

It's not reasonable to expect all callers of Git to intuitively know
where the Git distributor or user has configured these locations to
be, so add some entries to allow us to determine their location.  Honor
the GIT_ATTR_NOSYSTEM environment variable if one is specified.  Expose
the accessor functions in a way that we can reuse them from within the
var code.

In order to make our paths consistent on Windows and also use the same
form as paths use in "git rev-parse", let's normalize the path before we
return it.  This results in Windows-style paths that use slashes, which
is convenient for making our tests function in a consistent way across
platforms.  Note that this requires that some of our values be freed, so
let's add a flag about whether the value needs to be freed and use it
accordingly.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  6 ++++++
 builtin/var.c             | 29 +++++++++++++++++++++++++++++
 t/t0007-git-var.sh        | 20 ++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f0f647e14a..dfbe5cb52b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -74,6 +74,12 @@ GIT_DEFAULT_BRANCH::
 GIT_SHELL_PATH::
     The path of the binary providing the POSIX shell for commands which use the shell.
 
+GIT_ATTR_SYSTEM::
+    The path to the system linkgit:gitattributes[5] file, if one is enabled.
+
+GIT_ATTR_GLOBAL::
+    The path to the global (per-user) linkgit:gitattributes[5] file.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index ce6a2231ca..7cec05a49a 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -4,6 +4,7 @@
  * Copyright (C) Eric Biederman, 2005
  */
 #include "builtin.h"
+#include "attr.h"
 #include "config.h"
 #include "editor.h"
 #include "ident.h"
@@ -51,6 +52,26 @@ static char *shell_path(int flag)
 	return xstrdup(SHELL_PATH);
 }
 
+static char *git_attr_val_system(int flag)
+{
+	if (git_attr_system_is_enabled()) {
+		char *file = xstrdup(git_attr_system_file());
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static char *git_attr_val_global(int flag)
+{
+	char *file = xstrdup(git_attr_global_file());
+	if (file) {
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
 struct git_var {
 	const char *name;
 	char *(*read)(int);
@@ -84,6 +105,14 @@ static struct git_var git_vars[] = {
 		.name = "GIT_SHELL_PATH",
 		.read = shell_path,
 	},
+	{
+		.name = "GIT_ATTR_SYSTEM",
+		.read = git_attr_val_system,
+	},
+	{
+		.name = "GIT_ATTR_GLOBAL",
+		.read = git_attr_val_global,
+	},
 	{
 		.name = "",
 		.read = NULL,
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e35f07afcb..374d9f49e9 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -162,6 +162,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	esac
 '
 
+test_expect_success 'GIT_ATTR_SYSTEM produces expected output' '
+	test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM &&
+	(
+		sane_unset GIT_ATTR_NOSYSTEM &&
+		systempath=$(git var GIT_ATTR_SYSTEM) &&
+		test "$systempath" != ""
+	)
+'
+
+test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	globalpath=$(XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL) &&
+	test "$globalpath" = "$TRASHDIR/.config/git/attributes" &&
+	(
+		sane_unset XDG_CONFIG_HOME &&
+		globalpath=$(HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL) &&
+		test "$globalpath" = "$TRASHDIR/.config/git/attributes"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

Re: [PATCH v2 6/7] var: add attributes files locations

[Jeff King]

On Mon, Jun 26, 2023 at 07:00:07PM +0000, brian m. carlson wrote:

> @@ -51,6 +52,26 @@ static char *shell_path(int flag)
>  	return xstrdup(SHELL_PATH);
>  }
>  
> +static char *git_attr_val_system(int flag)
> +{
> +	if (git_attr_system_is_enabled()) {
> +		char *file = xstrdup(git_attr_system_file());
> +		normalize_path_copy(file, file);
> +		return file;
> +	}
> +	return NULL;
> +}

These new ones would ideally mark the "flag" variable with the UNUSED
attribute (in preparation for building with -Wunused-parameter).

I can also come through later and fix them up in a separate patch. It's
slightly awkward, just because I was about to post a patch that fixed
the existing functions in that file, and I'd have to either rebase on
top, or make a second pass once this is merged.

That said, I also renamed the "flag" variable in my patch because it's
super confusing (see my patch below for reference). So adjusting your
new callers to match (without my changes) would be a little weird. The
least-weird thing would be sticking my patch at the front of your
series, but I don't want to make you do extra work.

So I dunno. I'm mostly giving a heads-up, and seeing if you (or other
reviewers in the thread) have an idea to make this "flag" thing less
awful. I'm also happy to just do my topic separately, and then
eventually circle back after yours is merged.

-- >8 --
Subject: [PATCH] var: mark unused parameters in git_var callbacks

We abstract the set of variables into a table, with a "read" callback to
provide the value of each. Each callback takes a "flag" argument, but
most callbacks don't make use of it.

This flag is a bit odd. It may be set to IDENT_STRICT, which make sense
for ident-based callbacks, but is just confusing for things like
GIT_EDITOR.

At first glance, it seems like this is just a hack to let us directly
stick the generic git_committer_info() and git_author_info() functions
into our table. And we'd be better off to wrap them with local functions
which pass IDENT_STRICT, and have our callbacks take no option at all.

But that doesn't quite work. We pass IDENT_STRICT when the caller asks
for a specific variable, but otherwise do not (so that "git var -l" does
not bail if the committer ident cannot be formed).

So we really do need to pass in the flag to each invocation, even if the
individual callback doesn't care about it. Let's mark the unused ones so
that -Wunused-parameter does not complain. And while we're here, let's
rename them so that it's clear that the flag values we get will be from
the IDENT_* set. That may prevent confusion for future readers of the
code.

Another option would be to define our own local "strict" flag for the
callbacks, and then have wrappers that translate that to IDENT_STRICT
where it matters. But that would be more boilerplate for little gain
(most functions would still ignore the "strict" flag anyway).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/var.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 2149998980..10ee62f84c 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,17 +12,17 @@
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
-static const char *editor(int flag)
+static const char *editor(int ident_flag UNUSED)
 {
 	return git_editor();
 }
 
-static const char *sequence_editor(int flag)
+static const char *sequence_editor(int ident_flag UNUSED)
 {
 	return git_sequence_editor();
 }
 
-static const char *pager(int flag)
+static const char *pager(int ident_flag UNUSED)
 {
 	const char *pgm = git_pager(1);
 
@@ -31,7 +31,7 @@ static const char *pager(int flag)
 	return pgm;
 }
 
-static const char *default_branch(int flag)
+static const char *default_branch(int ident_flag UNUSED)
 {
 	return git_default_branch_name(1);
 }
-- 
2.41.0.490.g2678ffb796

Re: [PATCH v2 6/7] var: add attributes files locations

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

On 2023-06-27 at 07:05:57, Jeff King wrote:
> On Mon, Jun 26, 2023 at 07:00:07PM +0000, brian m. carlson wrote:
> 
> > @@ -51,6 +52,26 @@ static char *shell_path(int flag)
> >  	return xstrdup(SHELL_PATH);
> >  }
> >  
> > +static char *git_attr_val_system(int flag)
> > +{
> > +	if (git_attr_system_is_enabled()) {
> > +		char *file = xstrdup(git_attr_system_file());
> > +		normalize_path_copy(file, file);
> > +		return file;
> > +	}
> > +	return NULL;
> > +}
> 
> These new ones would ideally mark the "flag" variable with the UNUSED
> attribute (in preparation for building with -Wunused-parameter).
> 
> I can also come through later and fix them up in a separate patch. It's
> slightly awkward, just because I was about to post a patch that fixed
> the existing functions in that file, and I'd have to either rebase on
> top, or make a second pass once this is merged.
> 
> That said, I also renamed the "flag" variable in my patch because it's
> super confusing (see my patch below for reference). So adjusting your
> new callers to match (without my changes) would be a little weird. The
> least-weird thing would be sticking my patch at the front of your
> series, but I don't want to make you do extra work.
> 
> So I dunno. I'm mostly giving a heads-up, and seeing if you (or other
> reviewers in the thread) have an idea to make this "flag" thing less
> awful. I'm also happy to just do my topic separately, and then
> eventually circle back after yours is merged.

I've picked up your patch as the first patch in the series and will send
it out in v3 in just a few minutes.  Since I plan to have v3 be the last
round of this series, I'll let you send out any further changes as
fixups on top of that.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 6/7] var: add attributes files locations

[Junio C Hamano]

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

>> So I dunno. I'm mostly giving a heads-up, and seeing if you (or other
>> reviewers in the thread) have an idea to make this "flag" thing less
>> awful. I'm also happy to just do my topic separately, and then
>> eventually circle back after yours is merged.
>
> I've picked up your patch as the first patch in the series and will send
> it out in v3 in just a few minutes.  Since I plan to have v3 be the last
> round of this series, I'll let you send out any further changes as
> fixups on top of that.

Sounds like a sensible way to go, at least to me.  

Thanks for working well together.  I wish all conflicting topics
worked nicely with each other like these ;-)

Re: [PATCH v2 6/7] var: add attributes files locations

[Jeff King]

On Tue, Jun 27, 2023 at 04:12:27PM +0000, brian m. carlson wrote:

> > So I dunno. I'm mostly giving a heads-up, and seeing if you (or other
> > reviewers in the thread) have an idea to make this "flag" thing less
> > awful. I'm also happy to just do my topic separately, and then
> > eventually circle back after yours is merged.
> 
> I've picked up your patch as the first patch in the series and will send
> it out in v3 in just a few minutes.  Since I plan to have v3 be the last
> round of this series, I'll let you send out any further changes as
> fixups on top of that.

Thanks, that is optimal from my perspective. :)

The resulting series looks good to me, both the changes you integrated
from me, but also the whole thing overall. It is a little funny to stuff
the GIT_CONFIG_GLOBAL values into a string only to re-split them (versus
passing around a string_list itself), but I think it's a reasonable
compromise given the function interface.

-Peff

[PATCH v2 7/7] var: add config file locations

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Much like with attributes files, sometimes programs would like to know
the location of configuration files at the global or system levels.
However, it isn't always clear where these may live, especially for the
system file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

Since other parties cannot intuitively know how Git was compiled and
where it looks for these files, help them by providing variables that
can be queried.  Because we have multiple paths for global config
values, print them in order from highest to lowest priority, and be sure
to split on newlines so that "git var -l" produces two entries for the
global value.

However, be careful not to split all values on newlines, since our
editor values could well contain such characters, and we don't want to
split them in such a case.

Note in the documentation that some values may contain multiple paths
and that callers should be prepared for that fact.  This helps people
write code that will continue to work in the event we allow multiple
items elsewhere in the future.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt | 14 ++++++++
 builtin/var.c             | 68 ++++++++++++++++++++++++++++++++++++++-
 t/t0007-git-var.sh        | 65 +++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index dfbe5cb52b..c38fb3968b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -80,6 +80,20 @@ GIT_ATTR_SYSTEM::
 GIT_ATTR_GLOBAL::
     The path to the global (per-user) linkgit:gitattributes[5] file.
 
+GIT_CONFIG_SYSTEM::
+    The path to the system configuration file, if one is enabled.
+
+GIT_CONFIG_GLOBAL::
+    The path to the global (per-user) configuration files, if any.
+
+Most path values contain only one value. However, some can contain multiple
+values, which are separated by newlines, and are listed in order from highest to
+lowest priority.  Callers should be prepared for any such path value to contain
+multiple items.
+
+Note that paths are printed even if they do not exist, but not if they are
+disabled by other environment variables.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index 7cec05a49a..bd4e331312 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -72,50 +72,106 @@ static char *git_attr_val_global(int flag)
 	return NULL;
 }
 
+static char *git_config_val_system(int flag)
+{
+	if (git_config_system()) {
+		char *file = git_system_config();
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static char *git_config_val_global(int flag)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *user, *xdg;
+	size_t unused;
+
+	git_global_config(&user, &xdg);
+	if (xdg && *xdg) {
+		normalize_path_copy(xdg, xdg);
+		strbuf_addf(&buf, "%s\n", xdg);
+	}
+	if (user && *user) {
+		normalize_path_copy(user, user);
+		strbuf_addf(&buf, "%s\n", user);
+	}
+	free(xdg);
+	free(user);
+	strbuf_trim_trailing_newline(&buf);
+	if (buf.len == 0) {
+		strbuf_release(&buf);
+		return NULL;
+	}
+	return strbuf_detach(&buf, &unused);
+}
+
 struct git_var {
 	const char *name;
 	char *(*read)(int);
+	int multivalued;
 };
 static struct git_var git_vars[] = {
 	{
 		.name = "GIT_COMMITTER_IDENT",
 		.read = committer,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_AUTHOR_IDENT",
 		.read = author,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_EDITOR",
 		.read = editor,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_SEQUENCE_EDITOR",
 		.read = sequence_editor,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_PAGER",
 		.read = pager,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_DEFAULT_BRANCH",
 		.read = default_branch,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_SHELL_PATH",
 		.read = shell_path,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_ATTR_SYSTEM",
 		.read = git_attr_val_system,
+		.multivalued = 0,
 	},
 	{
 		.name = "GIT_ATTR_GLOBAL",
 		.read = git_attr_val_global,
+		.multivalued = 0,
+	},
+	{
+		.name = "GIT_CONFIG_SYSTEM",
+		.read = git_config_val_system,
+		.multivalued = 0,
+	},
+	{
+		.name = "GIT_CONFIG_GLOBAL",
+		.read = git_config_val_global,
+		.multivalued = 1,
 	},
 	{
 		.name = "",
 		.read = NULL,
+		.multivalued = 0,
 	},
 };
 
@@ -126,7 +182,17 @@ static void list_vars(void)
 
 	for (ptr = git_vars; ptr->read; ptr++)
 		if ((val = ptr->read(0))) {
-			printf("%s=%s\n", ptr->name, val);
+			if (ptr->multivalued && *val) {
+				struct string_list list = STRING_LIST_INIT_DUP;
+				int i;
+
+				string_list_split(&list, val, '\n', -1);
+				for (i = 0; i < list.nr; i++)
+					printf("%s=%s\n", ptr->name, list.items[i].string);
+				string_list_clear(&list, 0);
+			} else {
+				printf("%s=%s\n", ptr->name, val);
+			}
 			free(val);
 		}
 }
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 374d9f49e9..8cb597f99c 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -182,6 +182,49 @@ test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
 	)
 '
 
+test_expect_success 'GIT_CONFIG_SYSTEM points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	test_must_fail env GIT_CONFIG_NOSYSTEM=1 git var GIT_CONFIG_SYSTEM &&
+	(
+		sane_unset GIT_CONFIG_NOSYSTEM &&
+		systempath=$(git var GIT_CONFIG_SYSTEM) &&
+		test "$systempath" != "" &&
+		systempath=$(GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM) &&
+		if test_have_prereq MINGW
+		then
+			test "$systempath" = "nul"
+		else
+			test "$systempath" = "/dev/null"
+		fi &&
+		systempath=$(GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM) &&
+		test "$systempath" = "$TRASHDIR/gitconfig"
+	)
+'
+
+test_expect_success 'GIT_CONFIG_GLOBAL points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var GIT_CONFIG_GLOBAL >actual &&
+	echo "$TRASHDIR/foo/git/config" >expected &&
+	echo "$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected actual &&
+	(
+		sane_unset XDG_CONFIG_HOME &&
+		HOME="$TRASHDIR" git var GIT_CONFIG_GLOBAL >actual &&
+		echo "$TRASHDIR/.config/git/config" >expected &&
+		echo "$TRASHDIR/.gitconfig" >>expected &&
+		test_cmp expected actual &&
+		globalpath=$(GIT_CONFIG_GLOBAL=/dev/null git var GIT_CONFIG_GLOBAL) &&
+		if test_have_prereq MINGW
+		then
+			test "$globalpath" = "nul"
+		else
+			test "$globalpath" = "/dev/null"
+		fi &&
+		globalpath=$(GIT_CONFIG_GLOBAL="$TRASHDIR/gitconfig" git var GIT_CONFIG_GLOBAL) &&
+		test "$globalpath" = "$TRASHDIR/gitconfig"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.
@@ -199,6 +242,28 @@ test_expect_success 'git var -l lists config' '
 	test_cmp expect actual.bare
 '
 
+test_expect_success 'git var -l lists multiple global configs' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var -l >actual &&
+	grep "^GIT_CONFIG_GLOBAL=" actual >filtered &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/foo/git/config" >expected &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected filtered
+'
+
+test_expect_success 'git var -l does not split multiline editors' '
+	(
+		GIT_EDITOR="!f() {
+			echo Hello!
+		}; f" &&
+		export GIT_EDITOR &&
+		echo "GIT_EDITOR=$GIT_EDITOR" >expected &&
+		git var -l >var &&
+		sed -n -e "/^GIT_EDITOR/,\$p" var | head -n 3 >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'listing and asking for variables are exclusive' '
 	test_must_fail git var -l GIT_COMMITTER_IDENT
 '

Re: [PATCH v2 7/7] var: add config file locations

[Junio C Hamano]

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

>  struct git_var {
>  	const char *name;
>  	char *(*read)(int);
> +	int multivalued;
>  };
>  static struct git_var git_vars[] = {
>  	{
>  		.name = "GIT_COMMITTER_IDENT",
>  		.read = committer,
> +		.multivalued = 0,
>  	},
> ...
> +	{
> +		.name = "GIT_CONFIG_GLOBAL",
> +		.read = git_config_val_global,
> +		.multivalued = 1,
>  	},

As the majority of the variables are singletons and multi-valued
variables are expected to be in minority even in the future, we
would probably want to leave ".multivalued = 0" out of these array
elements and instead let the default zero initialization to take
place, which would make the resulting source less noisy.

Other than that, looks good.

Thanks.

[PATCH v3 0/8] Additional variables for git var

[brian m. carlson]

On many Unix systems, we have a good idea where Git's configuration
files and the shell it uses are located.  However, there are some
systems where that's not the case, such as Windows and with Homebrew,
where the expected files might be found in another location.

Right now, programs who would like to access things like the system
gitattributes or config file have to guess where the distributor of Git
placed these files, and with runtime relocation, it's not even
guaranteed that these will be in a fixed place from invocation to
invocation.  As a result, we need a way to query Git about the location
of these files.

This series introduces five new configuration variables that refer to
the shell path, the system and global gitattributes files, and the
system and global config files.  The global values are not technically
needed, since they should be computable from the environment alone, but
they are added to make life easier for users.

My intention is that this is the last round of this series, but if folks
have other comments, please let me know.

Changes since v2:
* Pick up Peff's patch.
* Rename function arguments.
* Mark unused function arguments as unused.

Changes since v1:
* Format variables with C99 initializers.
* Minimize use of temporary files in the tests.
* Remove debugging statement.
* Avoid grep where possible in tests.
* Duplicate memory rather than optionally choosing whether to free data.
* Add and document test_file_is_executable.
* Possibly more things which I have forgotten.

Jeff King (1):
  var: mark unused parameters in git_var callbacks

brian m. carlson (7):
  t: add a function to check executable bit
  var: add support for listing the shell
  var: format variable structure with C99 initializers
  var: adjust memory allocation for strings
  attr: expose and rename accessor functions
  var: add attributes files locations
  var: add config file locations

 Documentation/git-var.txt |  23 ++++++
 attr.c                    |  14 ++--
 attr.h                    |   9 ++
 builtin/var.c             | 167 +++++++++++++++++++++++++++++++++-----
 t/README                  |   6 ++
 t/t0007-git-var.sh        | 100 +++++++++++++++++++++++
 t/test-lib-functions.sh   |   9 ++
 7 files changed, 301 insertions(+), 27 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  5ed9f04f5b var: mark unused parameters in git_var callbacks
1:  20b7b85e98 = 2:  e7b676b6ae t: add a function to check executable bit
2:  7d92b4155f ! 3:  eae2f049cc var: add support for listing the shell
    @@ Documentation/git-var.txt: endif::git-default-pager[]
      linkgit:git-commit-tree[1]
     
      ## builtin/var.c ##
    -@@ builtin/var.c: static const char *default_branch(int flag)
    +@@ builtin/var.c: static const char *default_branch(int ident_flag UNUSED)
      	return git_default_branch_name(1);
      }
      
    -+static const char *shell_path(int flag)
    ++static const char *shell_path(int ident_flag UNUSED)
     +{
     +	return SHELL_PATH;
     +}
3:  29c338d59c = 4:  884f7a2967 var: format variable structure with C99 initializers
4:  2a2a762c44 ! 5:  c609b0a05c var: adjust memory allocation for strings
    @@ builtin/var.c
      
      static const char var_usage[] = "git var (-l | <variable>)";
      
    --static const char *editor(int flag)
    -+static char *committer(int flag)
    +-static const char *editor(int ident_flag UNUSED)
    ++static char *committer(int ident_flag)
      {
     -	return git_editor();
    -+	return xstrdup_or_null(git_committer_info(flag));
    ++	return xstrdup_or_null(git_committer_info(ident_flag));
      }
      
    --static const char *sequence_editor(int flag)
    -+static char *author(int flag)
    +-static const char *sequence_editor(int ident_flag UNUSED)
    ++static char *author(int ident_flag)
      {
     -	return git_sequence_editor();
    -+	return xstrdup_or_null(git_author_info(flag));
    ++	return xstrdup_or_null(git_author_info(ident_flag));
      }
      
    --static const char *pager(int flag)
    -+static char *editor(int flag)
    +-static const char *pager(int ident_flag UNUSED)
    ++static char *editor(int ident_flag UNUSED)
     +{
     +	return xstrdup_or_null(git_editor());
     +}
     +
    -+static char *sequence_editor(int flag)
    ++static char *sequence_editor(int ident_flag UNUSED)
     +{
     +	return xstrdup_or_null(git_sequence_editor());
     +}
     +
    -+static char *pager(int flag)
    ++static char *pager(int ident_flag UNUSED)
      {
      	const char *pgm = git_pager(1);
      
    @@ builtin/var.c
     +	return xstrdup(pgm);
      }
      
    --static const char *default_branch(int flag)
    -+static char *default_branch(int flag)
    +-static const char *default_branch(int ident_flag UNUSED)
    ++static char *default_branch(int ident_flag UNUSED)
      {
     -	return git_default_branch_name(1);
     +	return xstrdup_or_null(git_default_branch_name(1));
      }
      
    --static const char *shell_path(int flag)
    -+static char *shell_path(int flag)
    +-static const char *shell_path(int ident_flag UNUSED)
    ++static char *shell_path(int ident_flag UNUSED)
      {
     -	return SHELL_PATH;
     +	return xstrdup(SHELL_PATH);
5:  c0c5c59df9 = 6:  d029af8675 attr: expose and rename accessor functions
6:  49a04bd142 ! 7:  334df489f0 var: add attributes files locations
    @@ builtin/var.c
      #include "config.h"
      #include "editor.h"
      #include "ident.h"
    -@@ builtin/var.c: static char *shell_path(int flag)
    +@@ builtin/var.c: static char *shell_path(int ident_flag UNUSED)
      	return xstrdup(SHELL_PATH);
      }
      
    -+static char *git_attr_val_system(int flag)
    ++static char *git_attr_val_system(int ident_flag UNUSED)
     +{
     +	if (git_attr_system_is_enabled()) {
     +		char *file = xstrdup(git_attr_system_file());
    @@ builtin/var.c: static char *shell_path(int flag)
     +	return NULL;
     +}
     +
    -+static char *git_attr_val_global(int flag)
    ++static char *git_attr_val_global(int ident_flag UNUSED)
     +{
     +	char *file = xstrdup(git_attr_global_file());
     +	if (file) {
7:  a8b4d9396b ! 8:  abf079a923 var: add config file locations
    @@ Documentation/git-var.txt: GIT_ATTR_SYSTEM::
      linkgit:git-commit-tree[1]
     
      ## builtin/var.c ##
    -@@ builtin/var.c: static char *git_attr_val_global(int flag)
    +@@ builtin/var.c: static char *git_attr_val_global(int ident_flag UNUSED)
      	return NULL;
      }
      
    -+static char *git_config_val_system(int flag)
    ++static char *git_config_val_system(int ident_flag UNUSED)
     +{
     +	if (git_config_system()) {
     +		char *file = git_system_config();
    @@ builtin/var.c: static char *git_attr_val_global(int flag)
     +	return NULL;
     +}
     +
    -+static char *git_config_val_global(int flag)
    ++static char *git_config_val_global(int ident_flag UNUSED)
     +{
     +	struct strbuf buf = STRBUF_INIT;
     +	char *user, *xdg;
    @@ builtin/var.c: static char *git_attr_val_global(int flag)
      };
      static struct git_var git_vars[] = {
      	{
    - 		.name = "GIT_COMMITTER_IDENT",
    - 		.read = committer,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_AUTHOR_IDENT",
    - 		.read = author,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_EDITOR",
    - 		.read = editor,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_SEQUENCE_EDITOR",
    - 		.read = sequence_editor,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_PAGER",
    - 		.read = pager,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_DEFAULT_BRANCH",
    - 		.read = default_branch,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_SHELL_PATH",
    - 		.read = shell_path,
    -+		.multivalued = 0,
    - 	},
    - 	{
    - 		.name = "GIT_ATTR_SYSTEM",
    - 		.read = git_attr_val_system,
    -+		.multivalued = 0,
    - 	},
    - 	{
    +@@ builtin/var.c: static struct git_var git_vars[] = {
      		.name = "GIT_ATTR_GLOBAL",
      		.read = git_attr_val_global,
    -+		.multivalued = 0,
    -+	},
    + 	},
     +	{
     +		.name = "GIT_CONFIG_SYSTEM",
     +		.read = git_config_val_system,
    -+		.multivalued = 0,
     +	},
     +	{
     +		.name = "GIT_CONFIG_GLOBAL",
     +		.read = git_config_val_global,
     +		.multivalued = 1,
    - 	},
    ++	},
      	{
      		.name = "",
      		.read = NULL,
    -+		.multivalued = 0,
    - 	},
    - };
    - 
     @@ builtin/var.c: static void list_vars(void)
      
      	for (ptr = git_vars; ptr->read; ptr++)

[PATCH v3 1/8] var: mark unused parameters in git_var callbacks

[brian m. carlson]

From: Jeff King <peff@peff.net>

We abstract the set of variables into a table, with a "read" callback to
provide the value of each. Each callback takes a "flag" argument, but
most callbacks don't make use of it.

This flag is a bit odd. It may be set to IDENT_STRICT, which make sense
for ident-based callbacks, but is just confusing for things like
GIT_EDITOR.

At first glance, it seems like this is just a hack to let us directly
stick the generic git_committer_info() and git_author_info() functions
into our table. And we'd be better off to wrap them with local functions
which pass IDENT_STRICT, and have our callbacks take no option at all.

But that doesn't quite work. We pass IDENT_STRICT when the caller asks
for a specific variable, but otherwise do not (so that "git var -l" does
not bail if the committer ident cannot be formed).

So we really do need to pass in the flag to each invocation, even if the
individual callback doesn't care about it. Let's mark the unused ones so
that -Wunused-parameter does not complain. And while we're here, let's
rename them so that it's clear that the flag values we get will be from
the IDENT_* set. That may prevent confusion for future readers of the
code.

Another option would be to define our own local "strict" flag for the
callbacks, and then have wrappers that translate that to IDENT_STRICT
where it matters. But that would be more boilerplate for little gain
(most functions would still ignore the "strict" flag anyway).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/var.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 2149998980..10ee62f84c 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,17 +12,17 @@
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
-static const char *editor(int flag)
+static const char *editor(int ident_flag UNUSED)
 {
 	return git_editor();
 }
 
-static const char *sequence_editor(int flag)
+static const char *sequence_editor(int ident_flag UNUSED)
 {
 	return git_sequence_editor();
 }
 
-static const char *pager(int flag)
+static const char *pager(int ident_flag UNUSED)
 {
 	const char *pgm = git_pager(1);
 
@@ -31,7 +31,7 @@ static const char *pager(int flag)
 	return pgm;
 }
 
-static const char *default_branch(int flag)
+static const char *default_branch(int ident_flag UNUSED)
 {
 	return git_default_branch_name(1);
 }

[PATCH v3 2/8] t: add a function to check executable bit

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

In line with our other helper functions for paths, let's add a function
to check whether a path is executable, and if not, print a suitable
error message.  Document this function, and note that it must only be
used under the POSIXPERM prerequisite, since it doesn't otherwise work
on Windows.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 t/README                | 6 ++++++
 t/test-lib-functions.sh | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/t/README b/t/README
index bdfac4cceb..3e155011de 100644
--- a/t/README
+++ b/t/README
@@ -1098,6 +1098,12 @@ see test-lib-functions.sh for the full list and their options.
    the symbolic link in the file system and a part that does; then only
    the latter part need be protected by a SYMLINKS prerequisite (see below).
 
+ - test_path_is_executable
+
+   This tests whether a file is executable and prints an error message
+   if not. This must be used only under the POSIXPERM prerequisite
+   (see below).
+
  - test_oid_init
 
    This function loads facts and useful object IDs related to the hash
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b3864e22e9..2fa716c567 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -910,6 +910,15 @@ test_path_is_symlink () {
 	fi
 }
 
+test_path_is_executable () {
+	test "$#" -ne 1 && BUG "1 param"
+	if ! test -x "$1"
+	then
+		echo "$1 is not executable"
+		false
+	fi
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
 	test "$#" -ne 1 && BUG "1 param"

[PATCH v3 3/8] var: add support for listing the shell

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

On most Unix systems, finding a suitable shell is easy: one simply uses
"sh" with an appropriate PATH value.  However, in many Windows
environments, the shell is shipped alongside Git, and it may or may not
be in PATH, even if Git is.

In such an environment, it can be very helpful to query Git for the
shell it's using, since other tools may want to use the same shell as
well.  To help them out, let's add a variable, GIT_SHELL_PATH, that
points to the location of the shell.

On Unix, we know our shell must be executable to be functional, so
assume that the distributor has correctly configured their environment,
and use that as a basic test.  On Git for Windows, we know that our
shell will be one of a few fixed values, all of which end in "sh" (such
as "bash").  This seems like it might be a nice test on Unix as well,
since it is customary for all shells to end in "sh", but there probably
exist such systems that don't have such a configuration, so be careful
here not to break them.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  3 +++
 builtin/var.c             |  6 ++++++
 t/t0007-git-var.sh        | 15 +++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f40202b8e3..f0f647e14a 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -71,6 +71,9 @@ endif::git-default-pager[]
 GIT_DEFAULT_BRANCH::
     The name of the first branch created in newly initialized repositories.
 
+GIT_SHELL_PATH::
+    The path of the binary providing the POSIX shell for commands which use the shell.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index 10ee62f84c..bd340c5717 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -36,6 +36,11 @@ static const char *default_branch(int ident_flag UNUSED)
 	return git_default_branch_name(1);
 }
 
+static const char *shell_path(int ident_flag UNUSED)
+{
+	return SHELL_PATH;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -47,6 +52,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_SEQUENCE_EDITOR", sequence_editor },
 	{ "GIT_PAGER", pager },
 	{ "GIT_DEFAULT_BRANCH", default_branch },
+	{ "GIT_SHELL_PATH", shell_path },
 	{ "", NULL },
 };
 
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index eeb8539c1b..e35f07afcb 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -147,6 +147,21 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment
 	)
 '
 
+test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
+	shellpath=$(git var GIT_SHELL_PATH) &&
+	test_path_is_executable "$shellpath"
+'
+
+# We know in this environment that our shell will be one of a few fixed values
+# that all end in "sh".
+test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
+	shellpath=$(git var GIT_SHELL_PATH) &&
+	case "$shellpath" in
+	*sh) ;;
+	*) return 1;;
+	esac
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

[PATCH v3 4/8] var: format variable structure with C99 initializers

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, we have only two items in our variable struct.  However, in
the future, we're going to add two more items.  To help keep our diffs
nice and tidy and make this structure easier to read, switch to use
C99-style initializers for our data.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/var.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index bd340c5717..379564a399 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -46,14 +46,38 @@ struct git_var {
 	const char *(*read)(int);
 };
 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 },
-	{ "", NULL },
+	{
+		.name = "GIT_COMMITTER_IDENT",
+		.read = git_committer_info,
+	},
+	{
+		.name = "GIT_AUTHOR_IDENT",
+		.read = git_author_info,
+	},
+	{
+		.name = "GIT_EDITOR",
+		.read = editor,
+	},
+	{
+		.name = "GIT_SEQUENCE_EDITOR",
+		.read = sequence_editor,
+	},
+	{
+		.name = "GIT_PAGER",
+		.read = pager,
+	},
+	{
+		.name = "GIT_DEFAULT_BRANCH",
+		.read = default_branch,
+	},
+	{
+		.name = "GIT_SHELL_PATH",
+		.read = shell_path,
+	},
+	{
+		.name = "",
+		.read = NULL,
+	},
 };
 
 static void list_vars(void)

[PATCH v3 5/8] var: adjust memory allocation for strings

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, all of our values are constants whose allocation is managed
elsewhere.  However, in the future, we'll have some variables whose
memory we will need to free.  To keep things consistent, let's make each
of our functions allocate its own memory and make the caller responsible
for freeing it.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/var.c | 45 +++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 379564a399..d6f9f495c9 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,47 +12,57 @@
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
-static const char *editor(int ident_flag UNUSED)
+static char *committer(int ident_flag)
 {
-	return git_editor();
+	return xstrdup_or_null(git_committer_info(ident_flag));
 }
 
-static const char *sequence_editor(int ident_flag UNUSED)
+static char *author(int ident_flag)
 {
-	return git_sequence_editor();
+	return xstrdup_or_null(git_author_info(ident_flag));
 }
 
-static const char *pager(int ident_flag UNUSED)
+static char *editor(int ident_flag UNUSED)
+{
+	return xstrdup_or_null(git_editor());
+}
+
+static char *sequence_editor(int ident_flag UNUSED)
+{
+	return xstrdup_or_null(git_sequence_editor());
+}
+
+static char *pager(int ident_flag UNUSED)
 {
 	const char *pgm = git_pager(1);
 
 	if (!pgm)
 		pgm = "cat";
-	return pgm;
+	return xstrdup(pgm);
 }
 
-static const char *default_branch(int ident_flag UNUSED)
+static char *default_branch(int ident_flag UNUSED)
 {
-	return git_default_branch_name(1);
+	return xstrdup_or_null(git_default_branch_name(1));
 }
 
-static const char *shell_path(int ident_flag UNUSED)
+static char *shell_path(int ident_flag UNUSED)
 {
-	return SHELL_PATH;
+	return xstrdup(SHELL_PATH);
 }
 
 struct git_var {
 	const char *name;
-	const char *(*read)(int);
+	char *(*read)(int);
 };
 static struct git_var git_vars[] = {
 	{
 		.name = "GIT_COMMITTER_IDENT",
-		.read = git_committer_info,
+		.read = committer,
 	},
 	{
 		.name = "GIT_AUTHOR_IDENT",
-		.read = git_author_info,
+		.read = author,
 	},
 	{
 		.name = "GIT_EDITOR",
@@ -83,11 +93,13 @@ static struct git_var git_vars[] = {
 static void list_vars(void)
 {
 	struct git_var *ptr;
-	const char *val;
+	char *val;
 
 	for (ptr = git_vars; ptr->read; ptr++)
-		if ((val = ptr->read(0)))
+		if ((val = ptr->read(0))) {
 			printf("%s=%s\n", ptr->name, val);
+			free(val);
+		}
 }
 
 static const struct git_var *get_git_var(const char *var)
@@ -113,7 +125,7 @@ static int show_config(const char *var, const char *value, void *cb)
 int cmd_var(int argc, const char **argv, const char *prefix UNUSED)
 {
 	const struct git_var *git_var;
-	const char *val;
+	char *val;
 
 	if (argc != 2)
 		usage(var_usage);
@@ -134,6 +146,7 @@ int cmd_var(int argc, const char **argv, const char *prefix UNUSED)
 		return 1;
 
 	printf("%s\n", val);
+	free(val);
 
 	return 0;
 }

[PATCH v3 6/8] attr: expose and rename accessor functions

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Right now, the functions which determine the current system and global
gitattributes files are not exposed.  We'd like to use them in a future
commit, but they're not ideally named.  Rename them to something more
suitable as a public interface, expose them, and document them.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 attr.c | 14 +++++++-------
 attr.h |  9 +++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/attr.c b/attr.c
index d45d34058d..66203ce322 100644
--- a/attr.c
+++ b/attr.c
@@ -870,7 +870,7 @@ static struct attr_stack *read_attr(struct index_state *istate,
 	return res;
 }
 
-static const char *git_etc_gitattributes(void)
+const char *git_attr_system_file(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 *git_attr_global_file(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;
 }
 
-static int git_attr_system(void)
+int git_attr_system_is_enabled(void)
 {
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
@@ -920,14 +920,14 @@ static void bootstrap_attr_stack(struct index_state *istate,
 	push_stack(stack, e, NULL, 0);
 
 	/* system-wide frame */
-	if (git_attr_system()) {
-		e = read_attr_from_file(git_etc_gitattributes(), flags);
+	if (git_attr_system_is_enabled()) {
+		e = read_attr_from_file(git_attr_system_file(), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
 	/* home directory */
-	if (get_home_gitattributes()) {
-		e = read_attr_from_file(get_home_gitattributes(), flags);
+	if (git_attr_global_file()) {
+		e = read_attr_from_file(git_attr_global_file(), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
diff --git a/attr.h b/attr.h
index 676bd17ce2..2b745df405 100644
--- a/attr.h
+++ b/attr.h
@@ -227,4 +227,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction);
 
 void attr_start(void);
 
+/* Return the system gitattributes file. */
+const char *git_attr_system_file(void);
+
+/* Return the global gitattributes file, if any. */
+const char *git_attr_global_file(void);
+
+/* Return whether the system gitattributes file is enabled and should be used. */
+int git_attr_system_is_enabled(void);
+
 #endif /* ATTR_H */

[PATCH v3 7/8] var: add attributes files locations

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Currently, there are some programs which would like to read and parse
the gitattributes files at the global or system levels.  However, it's
not always obvious where these files live, especially for the system
file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

It's not reasonable to expect all callers of Git to intuitively know
where the Git distributor or user has configured these locations to
be, so add some entries to allow us to determine their location.  Honor
the GIT_ATTR_NOSYSTEM environment variable if one is specified.  Expose
the accessor functions in a way that we can reuse them from within the
var code.

In order to make our paths consistent on Windows and also use the same
form as paths use in "git rev-parse", let's normalize the path before we
return it.  This results in Windows-style paths that use slashes, which
is convenient for making our tests function in a consistent way across
platforms.  Note that this requires that some of our values be freed, so
let's add a flag about whether the value needs to be freed and use it
accordingly.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt |  6 ++++++
 builtin/var.c             | 29 +++++++++++++++++++++++++++++
 t/t0007-git-var.sh        | 20 ++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index f0f647e14a..dfbe5cb52b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -74,6 +74,12 @@ GIT_DEFAULT_BRANCH::
 GIT_SHELL_PATH::
     The path of the binary providing the POSIX shell for commands which use the shell.
 
+GIT_ATTR_SYSTEM::
+    The path to the system linkgit:gitattributes[5] file, if one is enabled.
+
+GIT_ATTR_GLOBAL::
+    The path to the global (per-user) linkgit:gitattributes[5] file.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index d6f9f495c9..79f7bdf55f 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -4,6 +4,7 @@
  * Copyright (C) Eric Biederman, 2005
  */
 #include "builtin.h"
+#include "attr.h"
 #include "config.h"
 #include "editor.h"
 #include "ident.h"
@@ -51,6 +52,26 @@ static char *shell_path(int ident_flag UNUSED)
 	return xstrdup(SHELL_PATH);
 }
 
+static char *git_attr_val_system(int ident_flag UNUSED)
+{
+	if (git_attr_system_is_enabled()) {
+		char *file = xstrdup(git_attr_system_file());
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static char *git_attr_val_global(int ident_flag UNUSED)
+{
+	char *file = xstrdup(git_attr_global_file());
+	if (file) {
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
 struct git_var {
 	const char *name;
 	char *(*read)(int);
@@ -84,6 +105,14 @@ static struct git_var git_vars[] = {
 		.name = "GIT_SHELL_PATH",
 		.read = shell_path,
 	},
+	{
+		.name = "GIT_ATTR_SYSTEM",
+		.read = git_attr_val_system,
+	},
+	{
+		.name = "GIT_ATTR_GLOBAL",
+		.read = git_attr_val_global,
+	},
 	{
 		.name = "",
 		.read = NULL,
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e35f07afcb..374d9f49e9 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -162,6 +162,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	esac
 '
 
+test_expect_success 'GIT_ATTR_SYSTEM produces expected output' '
+	test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM &&
+	(
+		sane_unset GIT_ATTR_NOSYSTEM &&
+		systempath=$(git var GIT_ATTR_SYSTEM) &&
+		test "$systempath" != ""
+	)
+'
+
+test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	globalpath=$(XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL) &&
+	test "$globalpath" = "$TRASHDIR/.config/git/attributes" &&
+	(
+		sane_unset XDG_CONFIG_HOME &&
+		globalpath=$(HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL) &&
+		test "$globalpath" = "$TRASHDIR/.config/git/attributes"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

[PATCH v3 8/8] var: add config file locations

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

Much like with attributes files, sometimes programs would like to know
the location of configuration files at the global or system levels.
However, it isn't always clear where these may live, especially for the
system file, which may have been hard-coded at compile time or computed
dynamically based on the runtime prefix.

Since other parties cannot intuitively know how Git was compiled and
where it looks for these files, help them by providing variables that
can be queried.  Because we have multiple paths for global config
values, print them in order from highest to lowest priority, and be sure
to split on newlines so that "git var -l" produces two entries for the
global value.

However, be careful not to split all values on newlines, since our
editor values could well contain such characters, and we don't want to
split them in such a case.

Note in the documentation that some values may contain multiple paths
and that callers should be prepared for that fact.  This helps people
write code that will continue to work in the event we allow multiple
items elsewhere in the future.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 Documentation/git-var.txt | 14 +++++++++
 builtin/var.c             | 57 +++++++++++++++++++++++++++++++++-
 t/t0007-git-var.sh        | 65 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index dfbe5cb52b..c38fb3968b 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -80,6 +80,20 @@ GIT_ATTR_SYSTEM::
 GIT_ATTR_GLOBAL::
     The path to the global (per-user) linkgit:gitattributes[5] file.
 
+GIT_CONFIG_SYSTEM::
+    The path to the system configuration file, if one is enabled.
+
+GIT_CONFIG_GLOBAL::
+    The path to the global (per-user) configuration files, if any.
+
+Most path values contain only one value. However, some can contain multiple
+values, which are separated by newlines, and are listed in order from highest to
+lowest priority.  Callers should be prepared for any such path value to contain
+multiple items.
+
+Note that paths are printed even if they do not exist, but not if they are
+disabled by other environment variables.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/builtin/var.c b/builtin/var.c
index 79f7bdf55f..ef45710a20 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -72,9 +72,45 @@ static char *git_attr_val_global(int ident_flag UNUSED)
 	return NULL;
 }
 
+static char *git_config_val_system(int ident_flag UNUSED)
+{
+	if (git_config_system()) {
+		char *file = git_system_config();
+		normalize_path_copy(file, file);
+		return file;
+	}
+	return NULL;
+}
+
+static char *git_config_val_global(int ident_flag UNUSED)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *user, *xdg;
+	size_t unused;
+
+	git_global_config(&user, &xdg);
+	if (xdg && *xdg) {
+		normalize_path_copy(xdg, xdg);
+		strbuf_addf(&buf, "%s\n", xdg);
+	}
+	if (user && *user) {
+		normalize_path_copy(user, user);
+		strbuf_addf(&buf, "%s\n", user);
+	}
+	free(xdg);
+	free(user);
+	strbuf_trim_trailing_newline(&buf);
+	if (buf.len == 0) {
+		strbuf_release(&buf);
+		return NULL;
+	}
+	return strbuf_detach(&buf, &unused);
+}
+
 struct git_var {
 	const char *name;
 	char *(*read)(int);
+	int multivalued;
 };
 static struct git_var git_vars[] = {
 	{
@@ -113,6 +149,15 @@ static struct git_var git_vars[] = {
 		.name = "GIT_ATTR_GLOBAL",
 		.read = git_attr_val_global,
 	},
+	{
+		.name = "GIT_CONFIG_SYSTEM",
+		.read = git_config_val_system,
+	},
+	{
+		.name = "GIT_CONFIG_GLOBAL",
+		.read = git_config_val_global,
+		.multivalued = 1,
+	},
 	{
 		.name = "",
 		.read = NULL,
@@ -126,7 +171,17 @@ static void list_vars(void)
 
 	for (ptr = git_vars; ptr->read; ptr++)
 		if ((val = ptr->read(0))) {
-			printf("%s=%s\n", ptr->name, val);
+			if (ptr->multivalued && *val) {
+				struct string_list list = STRING_LIST_INIT_DUP;
+				int i;
+
+				string_list_split(&list, val, '\n', -1);
+				for (i = 0; i < list.nr; i++)
+					printf("%s=%s\n", ptr->name, list.items[i].string);
+				string_list_clear(&list, 0);
+			} else {
+				printf("%s=%s\n", ptr->name, val);
+			}
 			free(val);
 		}
 }
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 374d9f49e9..8cb597f99c 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -182,6 +182,49 @@ test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' '
 	)
 '
 
+test_expect_success 'GIT_CONFIG_SYSTEM points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	test_must_fail env GIT_CONFIG_NOSYSTEM=1 git var GIT_CONFIG_SYSTEM &&
+	(
+		sane_unset GIT_CONFIG_NOSYSTEM &&
+		systempath=$(git var GIT_CONFIG_SYSTEM) &&
+		test "$systempath" != "" &&
+		systempath=$(GIT_CONFIG_SYSTEM=/dev/null git var GIT_CONFIG_SYSTEM) &&
+		if test_have_prereq MINGW
+		then
+			test "$systempath" = "nul"
+		else
+			test "$systempath" = "/dev/null"
+		fi &&
+		systempath=$(GIT_CONFIG_SYSTEM="$TRASHDIR/gitconfig" git var GIT_CONFIG_SYSTEM) &&
+		test "$systempath" = "$TRASHDIR/gitconfig"
+	)
+'
+
+test_expect_success 'GIT_CONFIG_GLOBAL points to the correct location' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var GIT_CONFIG_GLOBAL >actual &&
+	echo "$TRASHDIR/foo/git/config" >expected &&
+	echo "$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected actual &&
+	(
+		sane_unset XDG_CONFIG_HOME &&
+		HOME="$TRASHDIR" git var GIT_CONFIG_GLOBAL >actual &&
+		echo "$TRASHDIR/.config/git/config" >expected &&
+		echo "$TRASHDIR/.gitconfig" >>expected &&
+		test_cmp expected actual &&
+		globalpath=$(GIT_CONFIG_GLOBAL=/dev/null git var GIT_CONFIG_GLOBAL) &&
+		if test_have_prereq MINGW
+		then
+			test "$globalpath" = "nul"
+		else
+			test "$globalpath" = "/dev/null"
+		fi &&
+		globalpath=$(GIT_CONFIG_GLOBAL="$TRASHDIR/gitconfig" git var GIT_CONFIG_GLOBAL) &&
+		test "$globalpath" = "$TRASHDIR/gitconfig"
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.
@@ -199,6 +242,28 @@ test_expect_success 'git var -l lists config' '
 	test_cmp expect actual.bare
 '
 
+test_expect_success 'git var -l lists multiple global configs' '
+	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
+	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var -l >actual &&
+	grep "^GIT_CONFIG_GLOBAL=" actual >filtered &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/foo/git/config" >expected &&
+	echo "GIT_CONFIG_GLOBAL=$TRASHDIR/.gitconfig" >>expected &&
+	test_cmp expected filtered
+'
+
+test_expect_success 'git var -l does not split multiline editors' '
+	(
+		GIT_EDITOR="!f() {
+			echo Hello!
+		}; f" &&
+		export GIT_EDITOR &&
+		echo "GIT_EDITOR=$GIT_EDITOR" >expected &&
+		git var -l >var &&
+		sed -n -e "/^GIT_EDITOR/,\$p" var | head -n 3 >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'listing and asking for variables are exclusive' '
 	test_must_fail git var -l GIT_COMMITTER_IDENT
 '