threads / patch / 59899

patch, 3 partsAdditional variables for git var

Subject: [PATCH 0/3] Additional variables for git var

## tl;dr

42 messages between Jun 22, 2023 and Jun 27, 2023. Diffs are folded; open one to read it.

replies: 41people: 5as markdown or json

brian m. carlson· Jun 22, 2023, 19:50 UTC · lore

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

brian m. carlson· Jun 22, 2023, 19:50 UTC · re: brian m. carlson · lore

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

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(+)

Show changes to 3 files +21 −0

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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.

Eric Sunshine· Jun 22, 2023, 20:42 UTC · re: brian m. carlson · lore

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

On Thu, Jun 22, 2023 at 4:03 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 27 quoted lines

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

Show 6 quoted lines

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

Junio C Hamano· Jun 22, 2023, 21:05 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 7 quoted lines

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

Eric Sunshine· Jun 22, 2023, 21:13 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 22, 2023 at 5:05 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

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

brian m. carlson· Jun 22, 2023, 21:25 UTC · re: Junio C Hamano · lore

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

On 2023-06-22 at 21:05:39, Junio C Hamano wrote:

Show 6 quoted lines

> 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

Junio C Hamano· Jun 22, 2023, 21:41 UTC · re: brian m. carlson · lore

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

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

Show 17 quoted lines

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

brian m. carlson· Jun 22, 2023, 21:20 UTC · re: Eric Sunshine · lore

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

On 2023-06-22 at 20:42:31, Eric Sunshine wrote:

Show 7 quoted lines

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

Show 8 quoted lines

> 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

brian m. carlson· Jun 22, 2023, 19:50 UTC · re: brian m. carlson · lore

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

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

Show changes to 3 files +138 −10

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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
 '

Eric Sunshine· Jun 22, 2023, 21:35 UTC · re: brian m. carlson · lore

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

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 39 quoted lines

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

Show 11 quoted lines

> +       { "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 .)" &&

Show 16 quoted lines

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

Show 10 quoted lines

> @@ -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
> +       )
> +'

brian m. carlson· Jun 22, 2023, 19:50 UTC · re: brian m. carlson · lore

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

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

Show changes to 5 files +70 −11

Documentation/git-var.txt, attr.c, attr.h, builtin/var.c, t/t0007-git-var.sh

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.

Derrick Stolee· Jun 22, 2023, 20:19 UTC · re: brian m. carlson · lore

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

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

Show 5 quoted lines

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

Show 10 quoted lines

>  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

brian m. carlson· Jun 22, 2023, 21:17 UTC · re: Derrick Stolee · lore

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

On 2023-06-22 at 20:19:46, Derrick Stolee wrote:

Show 11 quoted lines

> 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

Junio C Hamano· Jun 22, 2023, 21:37 UTC · re: brian m. carlson · lore

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

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

Show 14 quoted lines

> 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 ;-)

Junio C Hamano· Jun 22, 2023, 21:17 UTC · re: brian m. carlson · lore

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

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

Show 17 quoted lines

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

These are sensible, but ...

Show 5 quoted lines

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

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

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

Show 24 quoted lines

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

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

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

Show 9 quoted lines

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

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

Show 18 quoted lines

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

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

Eric Sunshine· Jun 22, 2023, 21:18 UTC · re: brian m. carlson · lore

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

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 33 quoted lines

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

Show 5 quoted lines

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

And here too.

> +'

brian m. carlson· Jun 22, 2023, 21:30 UTC · re: Eric Sunshine · lore

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

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

Eric Sunshine· Jun 22, 2023, 21:21 UTC · re: brian m. carlson · lore

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

On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 28 quoted lines

> 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);

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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(+)

Show changes to 2 files +15 −0

t/README, t/test-lib-functions.sh

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"

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

Show changes to builtin/var.c +32 −8

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)

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

Show changes to builtin/var.c +29 −16

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

Junio C Hamano· Jun 26, 2023, 19:56 UTC · re: brian m. carlson · lore

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

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

Show 12 quoted lines

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

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

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(+)

Show changes to 3 files +24 −0

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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.

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

Show changes to 2 files +16 −7

attr.c, attr.h

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 */

Junio C Hamano· Jun 26, 2023, 19:58 UTC · re: brian m. carlson · lore

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

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

Show 12 quoted lines

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

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

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(+)

Show changes to 3 files +55 −0

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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.

Jeff King· Jun 27, 2023, 07:05 UTC · re: brian m. carlson · lore

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

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

Show 13 quoted lines

> @@ -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(-)

Show changes to builtin/var.c +4 −4

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

brian m. carlson· Jun 27, 2023, 16:12 UTC · re: Jeff King · lore

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

On 2023-06-27 at 07:05:57, Jeff King wrote:

Show 34 quoted lines

> 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

Junio C Hamano· Jun 27, 2023, 17:56 UTC · re: brian m. carlson · lore

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

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

Show 9 quoted lines

>> 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 ;-)

Jeff King· Jun 27, 2023, 20:16 UTC · re: brian m. carlson · lore

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

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

Show 9 quoted lines

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

brian m. carlson· Jun 26, 2023, 19:00 UTC · re: brian m. carlson · lore

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

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

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.

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

Show changes to 3 files +146 −1

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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
 '

Junio C Hamano· Jun 26, 2023, 20:02 UTC · re: brian m. carlson · lore

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

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

Show 17 quoted lines

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

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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.

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

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

Show changes to builtin/var.c +4 −4

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);
 }

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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

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

Show changes to 2 files +15 −0

t/README, t/test-lib-functions.sh

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"

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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

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

Show changes to builtin/var.c +32 −8

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)

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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

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(+)

Show changes to 3 files +24 −0

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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.

brian m. carlson· Jun 27, 2023, 16:18 UTC · re: brian m. carlson · lore

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

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

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

Show changes to builtin/var.c +29 −16

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

brian m. carlson· Jun 27, 2023, 16:19 UTC · re: brian m. carlson · lore

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

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

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.

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

Show changes to 3 files +135 −1

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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
 '

brian m. carlson· Jun 27, 2023, 16:19 UTC · re: brian m. carlson · lore

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

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

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(+)

Show changes to 3 files +55 −0

Documentation/git-var.txt, builtin/var.c, t/t0007-git-var.sh

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.

brian m. carlson· Jun 27, 2023, 16:19 UTC · re: brian m. carlson · lore

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

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

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

Show changes to 2 files +16 −7

attr.c, attr.h

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 */