threads / rfc / 24703

RFC patchAdd global and system-wide gitattributes

Subject: [PATCH/RFC] Add global and system-wide gitattributes

## tl;dr

36 messages between Aug 11, 2010 and Sep 2, 2010. Diffs are folded; open one to read it.

replies: 35people: 7as markdown or json

Petr Onderka· Aug 11, 2010, 01:04 UTC · lore

Allow gitattributes to be set globally and system wide in ~/.giattributes and $(prefix)/etc/gitattributes files, respectively. This way, settings for particular file types can be set in one place and apply for all user's repositories.

Contents of those two files are added to the attr_stack struct that contains content of info/attributes, so that prepare_attr_stack() keeps working as is and doesn't have to pop and then put back two more structs (in addition to the "info" one).

Some parts of the code were copied from the implementation of the same functionality in config.c.

Signed-off-by: Petr Onderka <gsvick@gmail.com>
---
Hi,

I thought this feature would be useful for me, so I coded it up. What do you think? Is it ready to be included to the official repository as is?

 Documentation/gitattributes.txt |    5 ++-
 Makefile                        |    6 +++++
 attr.c                          |   43 ++++++++++++++++++++++++++++++++------
 configure.ac                    |    7 ++++++
 4 files changed, 52 insertions(+), 9 deletions(-)
Show changes to 4 files +52 −9

Documentation/gitattributes.txt, Makefile, attr.c, configure.ac

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 564586b..351b014 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -7,7 +7,7 @@ gitattributes - defining attributes per path
 
 SYNOPSIS
 --------
-$GIT_DIR/info/attributes, .gitattributes
+$GIT_DIR/info/attributes, /etc/gitattributes, ~/.gitattributes, .gitattributes
 
 
 DESCRIPTION
@@ -58,7 +58,8 @@ attribute.  The rules how the pattern matches paths are the
 same as in `.gitignore` files; see linkgit:gitignore[5].
 
 When deciding what attributes are assigned to a path, git
-consults `$GIT_DIR/info/attributes` file (which has the highest
+consults `$GIT_DIR/info/attributes`, `~/.gitattributes`
+and `$(prefix)/etc/gitconfig` files (in order of decreasing
 precedence), `.gitattributes` file in the same directory as the
 path in question, and its parent directories up to the toplevel of the
 work tree (the further the directory that contains `.gitattributes`
diff --git a/Makefile b/Makefile
index bc3c570..eadd2d7 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,7 @@ STRIP ?= strip
 #   infodir
 #   htmldir
 #   ETC_GITCONFIG (but not sysconfdir)
+#   ETC_GITATTRIBUTES
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -286,9 +287,11 @@ htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 else
 sysconfdir = $(prefix)/etc
 ETC_GITCONFIG = etc/gitconfig
+ETC_GITATTRIBUTES = etc/gitattributes
 endif
 lib = lib
 # DESTDIR=
@@ -1502,6 +1505,7 @@ endif
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
@@ -1872,6 +1876,8 @@ builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 
 config.s config.o: EXTRA_CPPFLAGS = -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.s attr.o: EXTRA_CPPFLAGS = -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+
 http.s http.o: EXTRA_CPPFLAGS = -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
diff --git a/attr.c b/attr.c
index 8ba606c..b1c3a30 100644
--- a/attr.c
+++ b/attr.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "exec_cmd.h"
 #include "attr.h"
 
 const char git_attr__true[] = "(builtin)true";
@@ -326,16 +327,16 @@ static struct attr_stack *read_attr_from_array(const char **list)
 static enum git_attr_direction direction;
 static struct index_state *use_index;
 
-static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_file(const char *path, int macro_ok, struct attr_stack *res)
 {
 	FILE *fp = fopen(path, "r");
-	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
 	if (!fp)
-		return NULL;
-	res = xcalloc(1, sizeof(*res));
+		return res;
+	if (!res)
+		res = xcalloc(1, sizeof(*res));
 	while (fgets(buf, sizeof(buf), fp))
 		handle_attr_line(res, buf, path, ++lineno, macro_ok);
 	fclose(fp);
@@ -407,10 +408,10 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 	if (direction == GIT_ATTR_CHECKOUT) {
 		res = read_attr_from_index(path, macro_ok);
 		if (!res)
-			res = read_attr_from_file(path, macro_ok);
+			res = read_attr_from_file(path, macro_ok, NULL);
 	}
 	else if (direction == GIT_ATTR_CHECKIN) {
-		res = read_attr_from_file(path, macro_ok);
+		res = read_attr_from_file(path, macro_ok, NULL);
 		if (!res)
 			/*
 			 * There is no checked out .gitattributes file there, but
@@ -462,8 +463,28 @@ static void drop_attr_stack(void)
 	}
 }
 
+const char *git_etc_gitattributes(void)
+{
+	static const char *system_wide;
+	if (!system_wide)
+		system_wide = system_path(ETC_GITATTRIBUTES);
+	return system_wide;
+}
+
+int git_attr_system(void)
+{
+	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
+}
+
+int git_attr_global(void)
+{
+	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
+}
+
 static void bootstrap_attr_stack(void)
 {
+	const char *home;
+
 	if (!attr_stack) {
 		struct attr_stack *elem;
 
@@ -480,7 +501,15 @@ static void bootstrap_attr_stack(void)
 			debug_push(elem);
 		}
 
-		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);
+		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1, NULL);
+		home = get_home_directory();
+		if (git_attr_global() && home) {
+			char *user_attr = xstrdup(mkpath("%s/%s", home, GITATTRIBUTES_FILE));
+			elem = read_attr_from_file(user_attr, 1, elem);
+			free(user_attr);
+		}
+		if (git_attr_system())
+			elem = read_attr_from_file(git_etc_gitattributes(), 1, elem);
 		if (!elem)
 			elem = xcalloc(1, sizeof(*elem));
 		elem->origin = NULL;
diff --git a/configure.ac b/configure.ac
index 5601e8b..773b835 100644
--- a/configure.ac
+++ b/configure.ac
@@ -285,6 +285,13 @@ GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
 			If VALUE is not fully qualified it will be interpretted
 			as a path relative to the computed prefix at runtime.)
 
+# Allow user to set ETC_GITATTRIBUTS variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
+			Use VALUE instead of /etc/gitattributes as the
+			global git attributes file.
+			If VALUE is not fully qualified it will be interpretted
+			as a path relative to the computed prefix at runtime.)
+
 #
 # Allow user to set the default pager
 GIT_PARSE_WITH_SET_MAKE_VAR(pager, DEFAULT_PAGER,
-- 
1.7.1.msysgit.0.10.g9ffa0
Henrik Grubbström· Aug 11, 2010, 09:20 UTC · re: Petr Onderka · lore

Re: [PATCH/RFC] Add global and system-wide gitattributes

Hi.
On Wed, 11 Aug 2010, Petr Onderka wrote:
> Allow gitattributes to be set globally and system wide in ~/.giattributes
                                                                ^^
I assume you mean "~/.gitattributes" (ie missing a 't').
> and $(prefix)/etc/gitattributes files, respectively. This way, settings
> for particular file types can be set in one place and apply for all user's
> repositories.
[...]
Show 5 quoted lines
> Hi,
>
> I thought this feature would be useful for me, so I coded it up.
> What do you think? Is it ready to be included to the official 
> repository as is?
The feature as such seems reasonable.
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 564586b..351b014 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
[...]
Show 7 quoted lines
> When deciding what attributes are assigned to a path, git
> -consults `$GIT_DIR/info/attributes` file (which has the highest
> +consults `$GIT_DIR/info/attributes`, `~/.gitattributes`
> +and `$(prefix)/etc/gitconfig` files (in order of decreasing
> precedence), `.gitattributes` file in the same directory as the
> path in question, and its parent directories up to the toplevel of the
> work tree (the further the directory that contains `.gitattributes`

I'm not sure if the above priority order is the most desireable order, since the user may want to set a default attribute, and have it to be overridden by the .gitattributes in the repositories.

Show 6 quoted lines
> diff --git a/configure.ac b/configure.ac
> index 5601e8b..773b835 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -285,6 +285,13 @@ GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
> 			If VALUE is not fully qualified it will be interpretted
                                                                            ^^
Spello (not your fault): "interpreted".
Show 7 quoted lines
> 			as a path relative to the computed prefix at runtime.)
>
> +# Allow user to set ETC_GITATTRIBUTS variable
> +GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
> +			Use VALUE instead of /etc/gitattributes as the
> +			global git attributes file.
> +			If VALUE is not fully qualified it will be interpretted
                                                                            ^^
Same.
> +			as a path relative to the computed prefix at runtime.)
> +

-- Henrik Grubbström grubba@grubba.org Roxen Internet Software AB grubba@roxen.com

Petr Onderka· Aug 11, 2010, 10:50 UTC · re: Henrik Grubbström · lore

Re: [PATCH/RFC] Add global and system-wide gitattributes

Hi,
>> Allow gitattributes to be set globally and system wide in ~/.giattributes
>
>                                                               ^^
> I assume you mean "~/.gitattributes" (ie missing a 't').
Yeah.
> I'm not sure if the above priority order is the most desireable order, since
> the user may want to set a default attribute, and have it to be overridden
> by the .gitattributes in the repositories.

I wasn't sure either. I'm assuming info/gitattributes was made with higher priority, so that that users could override .gitattributes without polluting their repository, so I made the new ones behave the same way. But now that I think of it, you probably want to do that only for a specific repository, and have defaults for all repositories that could be overriden by local .gitattributes. I'm going to rewrite it this way.

Petr Onderka
Matthieu Moy· Aug 11, 2010, 12:31 UTC · re: Petr Onderka · lore

Re: [PATCH/RFC] Add global and system-wide gitattributes

Petr Onderka <gsvick@gmail.com> writes:
> Allow gitattributes to be set globally and system wide in ~/.giattributes
> and $(prefix)/etc/gitattributes files, respectively. This way, settings
> for particular file types can be set in one place and apply for all user's
> repositories.

The feature is definitely useful, and I'll use it as soon as it gets into git.git ;-).

One point: we need to make sure the choice for the the user-wide filename is the right one, since it's a pain to change it later.

We already have ~/.gitconfig, which often points to ~/.gitexcludes or ~/.gitignored (but this filename is specified with a config variable, for which we didn't manage to agree on a default value). We're about to add ~/.gitattributes. That makes 3 ~/.git* files, and I think it's time to make it a directory (similar to $GIT_DIR/info/)

I think it should be done like this:
1) Default core.excludesfile to $CONF/exclude
2) Make your user-wide gitattribute $CONF/attributes
3) Optionally, read $CONF/config as well as ~/.gitconfig so that
   people can have all their git configuration in the same directory.
Now, we need to agree on $CONF. Some non-options are:
* CONF=~/.gitconfig would clash with the file ~/.gitconfig
* CONF=~/.git would prevent people from versionning their $HOME.

I'd be in favor of following the freedesktop standard (roughly, defaulting to ~/.config/git):

  http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

because other applications start using it, and it allows one to easily keep the configuration in a (typically git-versionned) ~/.config directory. And if we are to choose a config directory, it doesn't harm to chose one consistant with other applications and with a standard.

I never got time to implement this. If you're willing to do something like that, that would be great. If not, I'd suggest having a config variable to point to the user-wide gitattributes file (without a default value), because it allows a future transition by giving a default value to the variable.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Aug 11, 2010, 22:19 UTC · re: Petr Onderka · lore

Re: [PATCH/RFC] Add global and system-wide gitattributes

Petr Onderka <gsvick@gmail.com> writes:
Show 14 quoted lines
> @@ -480,7 +501,15 @@ static void bootstrap_attr_stack(void)
>  			debug_push(elem);
>  		}
>  
> -		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);
> +		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1, NULL);
> +		home = get_home_directory();
> +		if (git_attr_global() && home) {
> +			char *user_attr = xstrdup(mkpath("%s/%s", home, GITATTRIBUTES_FILE));
> +			elem = read_attr_from_file(user_attr, 1, elem);
> +			free(user_attr);
> +		}
> +		if (git_attr_system())
> +			elem = read_attr_from_file(git_etc_gitattributes(), 1, elem);

Have you read the comment at the top of prepare-attr-stack? This patch feels triply wrong:

 - The attribute stack is arranged to have higher precedence file near the
   top ($GIT_DIR/info/attributes used to be the highest).  The above
   addition means that ~/.gitattributes from user's home trumps what is in
   a particular repository.  That is backwards.  You may work on more than
   one projects and have more than one repositories.  What you share among
   them personally will go to ~/.gitattributes, while a setting specific
   to a particular repository goes to $GIT_DIR/info/attributes and the
   latter needs to be able to override the former.
 - Same thing for git_attr_system() being at the end, which means you set
   up your own $GIT_DIR/info/attributes (or ~/.gitattributes) carefully
   but that can be broken by a selfish sysadmin who puts stuff that is
   only useful to him in /etc/gitattributes, which is not what you want.
 - Whenever we enter a new directory (either recursing into, or coming
   back up), prepare_attr_stack() is called to pop the attributes records
   from now-exited directories and push the attributes records from
   directories we are about to descend into.  The current code knows that
   the topmost element on the stack is special ($GIT_DIR/info/attributes)
   and first pops it, adjust the stack for elements that came from the
   directory hierarcy, and then pushes that back.  I don't see any code in
   the patch to do the equivalent for these two new attribute sources.
Petr Onderka· Aug 16, 2010, 16:51 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] Add global and system-wide gitattributes

Hi,
On Thu, Aug 12, 2010 at 00:19, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Have you read the comment at the top of prepare-attr-stack?  This patch
> feels triply wrong:
>
>  - The attribute stack is arranged to have higher precedence file near the
>   top ($GIT_DIR/info/attributes used to be the highest).  The above
>   addition means that ~/.gitattributes from user's home trumps what is in
>   a particular repository.  That is backwards.  You may work on more than
>   one projects and have more than one repositories.  What you share among
>   them personally will go to ~/.gitattributes, while a setting specific
>   to a particular repository goes to $GIT_DIR/info/attributes and the
>   latter needs to be able to override the former.
>
>  - Same thing for git_attr_system() being at the end, which means you set
>   up your own $GIT_DIR/info/attributes (or ~/.gitattributes) carefully
>   but that can be broken by a selfish sysadmin who puts stuff that is
>   only useful to him in /etc/gitattributes, which is not what you want.
>
>  - Whenever we enter a new directory (either recursing into, or coming
>   back up), prepare_attr_stack() is called to pop the attributes records
>   from now-exited directories and push the attributes records from
>   directories we are about to descend into.  The current code knows that
>   the topmost element on the stack is special ($GIT_DIR/info/attributes)
>   and first pops it, adjust the stack for elements that came from the
>   directory hierarcy, and then pushes that back.  I don't see any code in
>   the patch to do the equivalent for these two new attribute sources.

Yeah, I realize now that I got the precedence wrong, the corrected patch is in the following email.

But to your third point, I read the comment and the code worked, because I put all the attributes from the 3 files into one attr_stack struct (the one at the top of the stack). That's why I changed read_attr_from_file too. Of course this is irrelevant now that those attributes will be at the bottom of the stack.

Petr Onderka
Petr Onderka· Aug 16, 2010, 16:56 UTC · re: Petr Onderka · lore

[PATCH v2] Add global and system-wide gitattributes

Allow gitattributes to be set globally and system wide. This way, settings for particular file types can be set in one place and apply for all user's repositories.

The location of system-wide attributes file is $(prefix)/etc/gitattributes. The location of the global file can be configured by setting core.attributesfile.

Some parts of the code were copied from the implementation of the same functionality in config.c.

Signed-off-by: Petr Onderka <gsvick@gmail.com>
---
This version has the correct precedence for global and system-wide attributes.

I decided not to implement having user config files in a directory (at least for now). The location of the global attributes file can be set using core.attributesfile. This setting has no default, as Matthieu suggested.

 Documentation/config.txt        |    6 +++++
 Documentation/gitattributes.txt |   13 ++++++++--
 Makefile                        |    6 +++++
 attr.c                          |   44 ++++++++++++++++++++++++++++++++++++++-
 cache.h                         |    1 +
 config.c                        |    3 ++
 configure.ac                    |   10 ++++++++-
 environment.c                   |    1 +
 8 files changed, 79 insertions(+), 5 deletions(-)
Show changes to 8 files +79 −5

Documentation/config.txt, Documentation/gitattributes.txt, Makefile, attr.c, cache.h, config.c, configure.ac, environment.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f81fb91..e5034f4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.attributesfile::
+	In addition to '.gitattributes' (per-directory) and
+	'.git/info/attributes', git looks into this file for attributes
+	(see linkgit:gitattributes[5]). Path expansions are made the same
+	way as for `core.excludesfile`.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 564586b..c6bdeae 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -62,14 +62,21 @@ consults `$GIT_DIR/info/attributes` file (which has the highest
 precedence), `.gitattributes` file in the same directory as the
 path in question, and its parent directories up to the toplevel of the
 work tree (the further the directory that contains `.gitattributes`
-is from the path in question, the lower its precedence).
+is from the path in question, the lower its precedence). Finally
+global and system-wide files are considered (they have the lowest
+precedence).
 
 If you wish to affect only a single repository (i.e., to assign
-attributes to files that are particular to one user's workflow), then
+attributes to files that are particular to
+one user's workflow for that repository), then
 attributes should be placed in the `$GIT_DIR/info/attributes` file.
 Attributes which should be version-controlled and distributed to other
 repositories (i.e., attributes of interest to all users) should go into
-`.gitattributes` files.
+`.gitattributes` files. Attributes that should affect all repositories
+for a single user should be placed in a file specified by the
+`core.attributesfile` configuration option (see linkgit:git-config[1]).
+Attributes for all users on a system should be placed in the
+`$(prefix)/etc/gitattributes` file.
 
 Sometimes you would need to override an setting of an attribute
 for a path to `unspecified` state.  This can be done by listing
diff --git a/Makefile b/Makefile
index f33648d..3bfc483 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,7 @@ STRIP ?= strip
 #   infodir
 #   htmldir
 #   ETC_GITCONFIG (but not sysconfdir)
+#   ETC_GITATTRIBUTES
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -286,9 +287,11 @@ htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 else
 sysconfdir = $(prefix)/etc
 ETC_GITCONFIG = etc/gitconfig
+ETC_GITATTRIBUTES = etc/gitattributes
 endif
 lib = lib
 # DESTDIR=
@@ -1502,6 +1505,7 @@ endif
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
@@ -1872,6 +1876,8 @@ builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 
 config.s config.o: EXTRA_CPPFLAGS = -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.s attr.o: EXTRA_CPPFLAGS = -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+
 http.s http.o: EXTRA_CPPFLAGS = -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
diff --git a/attr.c b/attr.c
index 8ba606c..068e13b 100644
--- a/attr.c
+++ b/attr.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "exec_cmd.h"
 #include "attr.h"
 
 const char git_attr__true[] = "(builtin)true";
@@ -462,6 +463,24 @@ static void drop_attr_stack(void)
 	}
 }
 
+const char *git_etc_gitattributes(void)
+{
+	static const char *system_wide;
+	if (!system_wide)
+		system_wide = system_path(ETC_GITATTRIBUTES);
+	return system_wide;
+}
+
+int git_attr_system(void)
+{
+	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
+}
+
+int git_attr_global(void)
+{
+	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
+}
+
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -472,6 +491,27 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
+		if (git_attr_system()) {
+			elem = read_attr_from_file(git_etc_gitattributes(), 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
+		if (git_attr_global() && attributes_file) {
+			char *user_attr = xstrdup(attributes_file);
+
+			elem = read_attr_from_file(user_attr, 1);
+			free(user_attr);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
 		if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
 			elem = read_attr(GITATTRIBUTES_FILE, 1);
 			elem->origin = strdup("");
@@ -499,7 +539,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
-	 * set of attribute definitions.  Then, contents from
+	 * set of attribute definitions, followed by the contents
+	 * of $(prefix)/etc/gitattributes and a file specified by
+	 * core.attributesfile.  Then, contents from
 	 * .gitattribute files from directories closer to the
 	 * root to the ones in deeper directories are pushed
 	 * to the stack.  Finally, at the very top of the stack
diff --git a/cache.h b/cache.h
index c9fa3df..cd95801 100644
--- a/cache.h
+++ b/cache.h
@@ -1030,6 +1030,7 @@ extern int pager_use_color;
 
 extern const char *editor_program;
 extern const char *excludes_file;
+extern const char *attributes_file;
 
 /* base85 */
 int decode_85(char *dst, const char *line, int linelen);
diff --git a/config.c b/config.c
index cdcf583..f602cd4 100644
--- a/config.c
+++ b/config.c
@@ -563,6 +563,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.excludesfile"))
 		return git_config_pathname(&excludes_file, var, value);
 
+	if (!strcmp(var, "core.attributesfile"))
+		return git_config_pathname(&attributes_file, var, value);
+
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/configure.ac b/configure.ac
index 5601e8b..c5b3a41 100644
--- a/configure.ac
+++ b/configure.ac
@@ -282,7 +282,15 @@ GIT_PARSE_WITH(iconv))
 GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
 			Use VALUE instead of /etc/gitconfig as the
 			global git configuration file.
-			If VALUE is not fully qualified it will be interpretted
+			If VALUE is not fully qualified it will be interpreted
+			as a path relative to the computed prefix at runtime.)
+
+#
+# Allow user to set ETC_GITATTRIBUTES variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
+			Use VALUE instead of /etc/gitattributes as the
+			global git attributes file.
+			If VALUE is not fully qualified it will be interpreted
 			as a path relative to the computed prefix at runtime.)
 
 #
diff --git a/environment.c b/environment.c
index 83d38d3..58f719a 100644
--- a/environment.c
+++ b/environment.c
@@ -38,6 +38,7 @@ const char *pager_program;
 int pager_use_color = 1;
 const char *editor_program;
 const char *excludes_file;
+const char *attributes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 int read_replace_refs = 1;
 enum eol eol = EOL_UNSET;
-- 
1.7.2.1.2361.g5f8e3
Štěpán Němec· Aug 25, 2010, 09:55 UTC · re: Petr Onderka · lore

Re: [PATCH v2] Add global and system-wide gitattributes

Petr Onderka <gsvick@gmail.com> writes:
> Allow gitattributes to be set globally and system wide. This way, settings
> for particular file types can be set in one place and apply for all user's
> repositories.
[...]

I really can't wait till this gets into Git and I'm certainly not the only one (thank you so much for working on it, Petr!). Any reason the patch still hasn't been applied?

  Štěpán
Matthieu Moy· Aug 28, 2010, 17:33 UTC · re: Štěpán Němec · lore

Re: [PATCH v2] Add global and system-wide gitattributes

Štěpán Němec <stepnem@gmail.com> writes:
Show 10 quoted lines
> Petr Onderka <gsvick@gmail.com> writes:
>
>> Allow gitattributes to be set globally and system wide. This way, settings
>> for particular file types can be set in one place and apply for all user's
>> repositories.
>
> [...]
>
> I really can't wait till this gets into Git and I'm certainly not the
> only one (thank you so much for working on it, Petr!).
Same here ;-).
> Any reason the patch still hasn't been applied?
Usually, patches are applied if either
1) Junio has personal interest in it, or
2) Other people review the code and show interest for it.

I guess none of the conditions were met here (and Junio was very busy these days). I'm sure you can help about point 2) ;-)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Aug 30, 2010, 05:34 UTC · re: Matthieu Moy · lore

Re: [PATCH v2] Add global and system-wide gitattributes

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 22 quoted lines
> Štěpán Němec <stepnem@gmail.com> writes:
>
>> Petr Onderka <gsvick@gmail.com> writes:
>>
>>> Allow gitattributes to be set globally and system wide. This way, settings
>>> for particular file types can be set in one place and apply for all user's
>>> repositories.
>>
>> [...]
>>
>> I really can't wait till this gets into Git and I'm certainly not the
>> only one (thank you so much for working on it, Petr!).
>
> Same here ;-).
>
>> Any reason the patch still hasn't been applied?
>
> Usually, patches are applied if either
>
> 1) Junio has personal interest in it, or
>
> 2) Other people review the code and show interest for it.

(1) is a bit more subtle than that. Even when I do not foresee using a proposed feature myself, if I can imagine somebody else would benefit, and more importantly if I can see a healthy rationale behind the change so that I can convince myself to be an advocate of that change to the end users, such a change will fall into category (1). That is why I often ask people to justify their changes better than they originally do.

Isn't the patch in question on 'pu' to be tried by interested parties already?

Štěpán Němec· Aug 30, 2010, 09:09 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Add global and system-wide gitattributes

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Štěpán Němec <stepnem@gmail.com> writes:
>>
>>> Petr Onderka <gsvick@gmail.com> writes:
>>>
>>>> Allow gitattributes to be set globally and system wide. This way, settings
>>>> for particular file types can be set in one place and apply for all user's
>>>> repositories.
>>>
>>> [...]
>>>
>>> I really can't wait till this gets into Git and I'm certainly not the
>>> only one (thank you so much for working on it, Petr!).
>>
>> Same here ;-).
>>
>>> Any reason the patch still hasn't been applied?
>>
[...]
>
> Isn't the patch in question on 'pu' to be tried by interested parties
> already?

I see it now, sorry for the confusion. I didn't check when I saw no response to the mail.

Štěpán
Matthieu Moy· Aug 28, 2010, 18:35 UTC · re: Petr Onderka · lore

Re: [PATCH v2] Add global and system-wide gitattributes

Petr Onderka <gsvick@gmail.com> writes:
Show 7 quoted lines
> Allow gitattributes to be set globally and system wide. This way, settings
> for particular file types can be set in one place and apply for all user's
> repositories.
>
> The location of system-wide attributes file is $(prefix)/etc/gitattributes.
> The location of the global file can be configured by setting
> core.attributesfile.
Good, I like this :-)
> Some parts of the code were copied from the implementation of the same
> functionality in config.c.
(maybe you could say quickly which ones and why)
> +		if (git_attr_global() && attributes_file) {

I tried this, and attributes_file was NULL here. I don't know how it worked for you, but there should be a call to git_config here.

> +			char *user_attr = xstrdup(attributes_file);
> +
> +			elem = read_attr_from_file(user_attr, 1);
> +			free(user_attr);
Any reason for this xstrdup/free?

Patch follows for these two points + a test, to be squashed into yours.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Aug 28, 2010, 18:41 UTC · re: Matthieu Moy · lore

[PATCH] core.attributesfile: a fix, a simplification, and a test

Patch meant to be squashed into the core.attributesfile.
* attributes_file won't be set unless one calls git_config before => do
  this.
* There was a useless xstrdup/free in the code.
* This really deserves a test, so I added one in t0003-attributes.sh.
  (I've been too lazy to check the system-wide attributes file, though)
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 attr.c                |    6 ++----
 t/t0003-attributes.sh |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
Show changes to 2 files +16 −4

attr.c, t/t0003-attributes.sh

diff --git a/attr.c b/attr.c
index 068e13b..342791a 100644
--- a/attr.c
+++ b/attr.c
@@ -500,11 +500,9 @@ static void bootstrap_attr_stack(void)
 			}
 		}
 
+		git_config(git_default_config, NULL);
 		if (git_attr_global() && attributes_file) {
-			char *user_attr = xstrdup(attributes_file);
-
-			elem = read_attr_from_file(user_attr, 1);
-			free(user_attr);
+			elem = read_attr_from_file(attributes_file, 1);
 			if (elem) {
 				elem->origin = NULL;
 				elem->prev = attr_stack;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index de38c7f..24286e5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -15,6 +15,7 @@ attr_check () {
 
 }
 
+HOME=$(pwd)
 
 test_expect_success 'setup' '
 
@@ -36,6 +37,9 @@ test_expect_success 'setup' '
 		echo "d/* test=a/b/d/*"
 		echo "d/yes notest"
 	) >a/b/.gitattributes
+	(
+		echo "global test=global"
+	) >$HOME/global-gitattributes
 
 '
 
@@ -57,6 +61,16 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_success 'core.attributesfile' '
+	attr_check global unspecified &&
+	git config core.attributesfile "$HOME/global-gitattributes" &&
+	attr_check global global &&
+	git config core.attributesfile "~/global-gitattributes" &&
+	attr_check global global &&
+	echo "global test=precedence" >> .gitattributes &&
+	attr_check global precedence
+'
+
 test_expect_success 'attribute test: read paths from stdin' '
 
 	cat <<EOF > expect
-- 
1.7.2.2.175.ga619d.dirty
Štěpán Němec· Aug 29, 2010, 10:32 UTC · re: Matthieu Moy · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Thanks, Matthieu!

Petr's original patch doesn't apply cleanly due to a bogus context line after a recent change in Makefile. For convenience, below is the updated patch including Matthieu's changes applicable to current master.

And FWIW (I didn't try to build the documentation, though):
Tested-by: Štěpán Němec <stepnem@gmail.com>
--- 8< ---
From b63a6449dbf2c6d4a111ad840ae7d408afdd5e43 Mon Sep 17 00:00:00 2001
From: Petr Onderka <gsvick@gmail.com>
Date: Mon, 16 Aug 2010 16:56:53 +0000
Subject: [PATCH] Add global and system-wide gitattributes

Allow gitattributes to be set globally and system wide. This way, settings for particular file types can be set in one place and apply for all user's repositories.

The location of system-wide attributes file is $(prefix)/etc/gitattributes. The location of the global file can be configured by setting core.attributesfile.

Some parts of the code were copied from the implementation of the same functionality in config.c.

Signed-off-by: Petr Onderka <gsvick@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/config.txt        |    6 +++++
 Documentation/gitattributes.txt |   13 +++++++++--
 Makefile                        |    6 +++++
 attr.c                          |   42 ++++++++++++++++++++++++++++++++++++++-
 cache.h                         |    1 +
 config.c                        |    3 ++
 configure.ac                    |   10 ++++++++-
 environment.c                   |    1 +
 t/t0003-attributes.sh           |   14 +++++++++++++
 9 files changed, 91 insertions(+), 5 deletions(-)
Show changes to 9 files +91 −5

Documentation/config.txt, Documentation/gitattributes.txt, Makefile, attr.c, cache.h, config.c, configure.ac, environment.c, t/t0003-attributes.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 05ec3fe..0e15e72 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.attributesfile::
+	In addition to '.gitattributes' (per-directory) and
+	'.git/info/attributes', git looks into this file for attributes
+	(see linkgit:gitattributes[5]). Path expansions are made the same
+	way as for `core.excludesfile`.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2e2370c..ebd4852 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -62,14 +62,21 @@ consults `$GIT_DIR/info/attributes` file (which has the highest
 precedence), `.gitattributes` file in the same directory as the
 path in question, and its parent directories up to the toplevel of the
 work tree (the further the directory that contains `.gitattributes`
-is from the path in question, the lower its precedence).
+is from the path in question, the lower its precedence). Finally
+global and system-wide files are considered (they have the lowest
+precedence).
 
 If you wish to affect only a single repository (i.e., to assign
-attributes to files that are particular to one user's workflow), then
+attributes to files that are particular to
+one user's workflow for that repository), then
 attributes should be placed in the `$GIT_DIR/info/attributes` file.
 Attributes which should be version-controlled and distributed to other
 repositories (i.e., attributes of interest to all users) should go into
-`.gitattributes` files.
+`.gitattributes` files. Attributes that should affect all repositories
+for a single user should be placed in a file specified by the
+`core.attributesfile` configuration option (see linkgit:git-config[1]).
+Attributes for all users on a system should be placed in the
+`$(prefix)/etc/gitattributes` file.
 
 Sometimes you would need to override an setting of an attribute
 for a path to `unspecified` state.  This can be done by listing
diff --git a/Makefile b/Makefile
index b4745a5..fdb7b4e 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,7 @@ STRIP ?= strip
 #   infodir
 #   htmldir
 #   ETC_GITCONFIG (but not sysconfdir)
+#   ETC_GITATTRIBUTES
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -286,9 +287,11 @@ htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 else
 sysconfdir = $(prefix)/etc
 ETC_GITCONFIG = etc/gitconfig
+ETC_GITATTRIBUTES = etc/gitattributes
 endif
 lib = lib
 # DESTDIR=
@@ -1502,6 +1505,7 @@ endif
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
@@ -1873,6 +1877,8 @@ builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 
 config.s config.o: EXTRA_CPPFLAGS = -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.s attr.o: EXTRA_CPPFLAGS = -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+
 http.s http.o: EXTRA_CPPFLAGS = -DGIT_HTTP_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
diff --git a/attr.c b/attr.c
index 8ba606c..342791a 100644
--- a/attr.c
+++ b/attr.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "exec_cmd.h"
 #include "attr.h"
 
 const char git_attr__true[] = "(builtin)true";
@@ -462,6 +463,24 @@ static void drop_attr_stack(void)
 	}
 }
 
+const char *git_etc_gitattributes(void)
+{
+	static const char *system_wide;
+	if (!system_wide)
+		system_wide = system_path(ETC_GITATTRIBUTES);
+	return system_wide;
+}
+
+int git_attr_system(void)
+{
+	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
+}
+
+int git_attr_global(void)
+{
+	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
+}
+
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -472,6 +491,25 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
+		if (git_attr_system()) {
+			elem = read_attr_from_file(git_etc_gitattributes(), 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
+		git_config(git_default_config, NULL);
+		if (git_attr_global() && attributes_file) {
+			elem = read_attr_from_file(attributes_file, 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
 		if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
 			elem = read_attr(GITATTRIBUTES_FILE, 1);
 			elem->origin = strdup("");
@@ -499,7 +537,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
-	 * set of attribute definitions.  Then, contents from
+	 * set of attribute definitions, followed by the contents
+	 * of $(prefix)/etc/gitattributes and a file specified by
+	 * core.attributesfile.  Then, contents from
 	 * .gitattribute files from directories closer to the
 	 * root to the ones in deeper directories are pushed
 	 * to the stack.  Finally, at the very top of the stack
diff --git a/cache.h b/cache.h
index eb77e1d..28d9497 100644
--- a/cache.h
+++ b/cache.h
@@ -1033,6 +1033,7 @@ extern int pager_use_color;
 
 extern const char *editor_program;
 extern const char *excludes_file;
+extern const char *attributes_file;
 
 /* base85 */
 int decode_85(char *dst, const char *line, int linelen);
diff --git a/config.c b/config.c
index cdcf583..f602cd4 100644
--- a/config.c
+++ b/config.c
@@ -563,6 +563,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.excludesfile"))
 		return git_config_pathname(&excludes_file, var, value);
 
+	if (!strcmp(var, "core.attributesfile"))
+		return git_config_pathname(&attributes_file, var, value);
+
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/configure.ac b/configure.ac
index 5601e8b..c5b3a41 100644
--- a/configure.ac
+++ b/configure.ac
@@ -282,7 +282,15 @@ GIT_PARSE_WITH(iconv))
 GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
 			Use VALUE instead of /etc/gitconfig as the
 			global git configuration file.
-			If VALUE is not fully qualified it will be interpretted
+			If VALUE is not fully qualified it will be interpreted
+			as a path relative to the computed prefix at runtime.)
+
+#
+# Allow user to set ETC_GITATTRIBUTES variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
+			Use VALUE instead of /etc/gitattributes as the
+			global git attributes file.
+			If VALUE is not fully qualified it will be interpreted
 			as a path relative to the computed prefix at runtime.)
 
 #
diff --git a/environment.c b/environment.c
index 83d38d3..58f719a 100644
--- a/environment.c
+++ b/environment.c
@@ -38,6 +38,7 @@ const char *pager_program;
 int pager_use_color = 1;
 const char *editor_program;
 const char *excludes_file;
+const char *attributes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 int read_replace_refs = 1;
 enum eol eol = EOL_UNSET;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 53bd7fc..7fe3b49 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -15,6 +15,7 @@ attr_check () {
 
 }
 
+HOME=$(pwd)
 
 test_expect_success 'setup' '
 
@@ -36,6 +37,9 @@ test_expect_success 'setup' '
 		echo "d/* test=a/b/d/*"
 		echo "d/yes notest"
 	) >a/b/.gitattributes
+	(
+		echo "global test=global"
+	) >$HOME/global-gitattributes
 
 '
 
@@ -57,6 +61,16 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_success 'core.attributesfile' '
+	attr_check global unspecified &&
+	git config core.attributesfile "$HOME/global-gitattributes" &&
+	attr_check global global &&
+	git config core.attributesfile "~/global-gitattributes" &&
+	attr_check global global &&
+	echo "global test=precedence" >> .gitattributes &&
+	attr_check global precedence
+'
+
 test_expect_success 'attribute test: read paths from stdin' '
 
 	cat <<EOF > expect
-- 
1.7.1
Junio C Hamano· Aug 30, 2010, 08:04 UTC · re: Štěpán Němec · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Štěpán Němec <stepnem@gmail.com> writes:
Show 9 quoted lines
> Thanks, Matthieu!
>
> Petr's original patch doesn't apply cleanly due to a bogus context line
> after a recent change in Makefile. For convenience, below is the updated
> patch including Matthieu's changes applicable to current master.
>
> And FWIW (I didn't try to build the documentation, though):
>
> Tested-by: Štěpán Němec <stepnem@gmail.com>
Thanks, but this seems to break t8005 for whatever reason.
Show 9 quoted lines
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index 53bd7fc..7fe3b49 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -15,6 +15,7 @@ attr_check () {
>  
>  }
>  
> +HOME=$(pwd)

I see a few tests here and there that uses the test-trash directory as HOME; perhaps we should have done that in test-lib.sh to make our life easier?

Matthieu Moy· Aug 30, 2010, 08:26 UTC · re: Junio C Hamano · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Štěpán Němec <stepnem@gmail.com> writes:
>
>> Thanks, Matthieu!
>>
>> Petr's original patch doesn't apply cleanly due to a bogus context line
>> after a recent change in Makefile. For convenience, below is the updated
>> patch including Matthieu's changes applicable to current master.
>>
>> And FWIW (I didn't try to build the documentation, though):
>>
>> Tested-by: Štěpán Němec <stepnem@gmail.com>
>
> Thanks, but this seems to break t8005 for whatever reason.
The guilty line is
  git_config(git_default_config, NULL);
(t8005 passes if I remove it).

I don't understand why this breaks the test. It seems blame --encoding=UTF-8 relies on the fact that the i18n section of the configuration is not loaded.

An obvious fix on our side is to squash the patch below into the previous ones, to make sure the attributes code loads only the core configuration. OTOH, it seems to me that the blame code is very fragile, but I'm not familiar with this code to say whether and how it should be fixed.

Any other thoughts?
Show changes to 3 files +10 −1

attr.c, cache.h, config.c

diff --git a/attr.c b/attr.c
index 342791a..793a835 100644
--- a/attr.c
+++ b/attr.c
@@ -500,7 +500,7 @@ static void bootstrap_attr_stack(void)
                        }
                }
 
-               git_config(git_default_config, NULL);
+               git_config(git_core_config, NULL);
                if (git_attr_global() && attributes_file) {
                        elem = read_attr_from_file(attributes_file, 1);
                        if (elem) {
diff --git a/cache.h b/cache.h
index 28d9497..2d0bfa0 100644
--- a/cache.h
+++ b/cache.h
@@ -972,6 +972,7 @@ extern int update_server_info(int);
 
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
+extern int git_core_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern int git_config_parse_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
diff --git a/config.c b/config.c
index f602cd4..a2d1df9 100644
--- a/config.c
+++ b/config.c
@@ -602,6 +602,14 @@ static int git_default_core_config(const char *var, const char *value)
        return 0;
 }
 
+/* 
+ * Wrapper around git_default_core_config, with type acceptable as
+ * argument to git_config(...)
+ */
+int git_core_config(const char *var, const char *value, void *dummy) {
+       return git_default_core_config(var, value);
+}
+
 static int git_default_user_config(const char *var, const char *value)
 {
        if (!strcmp(var, "user.name")) {
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Aug 30, 2010, 20:47 UTC · re: Matthieu Moy · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> I don't understand why this breaks the test. It seems blame
> --encoding=UTF-8 relies on the fact that the i18n section of the
> configuration is not loaded.

That's interesting; I haven't traced the codepath involved, but I do not think "configuration is not loaded" is the issue. "Reading either before the main codepath is ready, or more likely overwriting/destroying what the main codepath has read it by re-reading the configuration" may be.

Perhaps the part that reads encoding configuration is busted and is not expected to be called way early or way late. In short, this patch introduces uncertainty that config reader is called at a random and unexpected (from the existing code's point of view) place in the codepath, and I wouldn't be very surprised if there are similar breakages introduced by it.

What does the callchain look like when we bootstrap the attr stack for the first time with this patch applied? Have we already located where the git repository is? Has the main codepath that wants to read encoding settings read them? Has the main codepath already used the command line option that overrides the settings it obtained from the configuration file? Is the extra reading of the config destroying that data?

I am afraid that your patch to narrow the parts of the config that is read from this codepath is only sweeping the problem under the rug, making the issue harder to diagnose. Wouldn't we see exactly the same issue if some codepath (other than blame) wants to see a core.* configuration not to be read in this codepath for whatever reason?

Junio C Hamano· Aug 30, 2010, 21:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> I don't understand why this breaks the test. It seems blame
>> --encoding=UTF-8 relies on the fact that the i18n section of the
>> configuration is not loaded.
>
> That's interesting; I haven't traced the codepath involved, but I do not
> think "configuration is not loaded" is the issue. "Reading either before
> the main codepath is ready, or more likely overwriting/destroying what the
> main codepath has read it by re-reading the configuration" may be.

I think that hunch is correct. A typical way we default to hardcoded value, overridable by configuration file, and then further use command line to override that, is for the main codepath to do the following in this order:

 - call git_config(git_appropriate_config); this changes the variables
   (with possibly hardcoded default) defined in environment.c;
 - parse command line options and override the variable;
 - use the variable at runtime.

This obviously relies on the main codepath having _total_ control of the calls made to git_config(). If you call git_config(git_default_config) when an attribute is asked for for the first time (which would be way after all of the above happened) behind the main codepath's back, you will of course break things. In the "blame" case, aren't you stomping on git_log_output_encoding?

The correct solution would be twofold, but the latter is rather painful:
 - The call from the bootstrap_attr_stack should use a callback that reads
   only the attribute file location configuration and _nothing else_.
   Also I do not think the parsing of this configuration variable needs to
   be in git_default_config() to begin with, if you are reading it from
   the bootstrap codepath on demand anyway, and not relying on the main
   codepath of any particular program to be calling on git_config().
 - The way programs (this is not limited to blame and other rev-list
   machinery users) implement the "use configured values but let command
   line override them" need to be changed.
   
   One possibility is to copy the values determined by reading the config
   and the command line to their own variables, so that later random call
   to git_config() won't stomp on the actual values to be used.  This is
   painful as environment.c variables are _meant_ to be easily usable as
   global variables and copying them away (which means they now need to be
   passed around throughout the callchain in the various APIs) defeats
   the whole point of having them.
Matthieu Moy· Aug 30, 2010, 22:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v3?] Add global and system-wide gitattributes

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> I don't understand why this breaks the test. It seems blame
>>> --encoding=UTF-8 relies on the fact that the i18n section of the
>>> configuration is not loaded.
>>
>> That's interesting; I haven't traced the codepath involved, but I do not
>> think "configuration is not loaded" is the issue. "Reading either before
>> the main codepath is ready, or more likely overwriting/destroying what the
>> main codepath has read it by re-reading the configuration" may be.
>
> I think that hunch is correct.
Confirmed.
Show 10 quoted lines
> A typical way we default to hardcoded value, overridable by
> configuration file, and then further use command line to override
> that, is for the main codepath to do the following in this order:
>
>  - call git_config(git_appropriate_config); this changes the variables
>    (with possibly hardcoded default) defined in environment.c;
>
>  - parse command line options and override the variable;
>
>  - use the variable at runtime.
Yes, this is the problem, with git_log_output_encoding as you guessed.
> The correct solution would be twofold, but the latter is rather painful:

Not that much in the case of git_log_output_encoding, but other uses of the same pattern may exist.

>  - The call from the bootstrap_attr_stack should use a callback that reads
>    only the attribute file location configuration and _nothing else_.
[...]
>  - The way programs (this is not limited to blame and other rev-list
>    machinery users) implement the "use configured values but let command
>    line override them" need to be changed.

I think it's reasonable to do both. Having both git_config() and command-line parsing write to the same variable is fragile and should be avoided IMHO, but OTOH, arbitrary calls to git_config(git_default_config) may break other things, so ...

Show 7 quoted lines
>    One possibility is to copy the values determined by reading the config
>    and the command line to their own variables, so that later random call
>    to git_config() won't stomp on the actual values to be used.  This is
>    painful as environment.c variables are _meant_ to be easily usable as
>    global variables and copying them away (which means they now need to be
>    passed around throughout the callchain in the various APIs) defeats
>    the whole point of having them.

I just keep two global variables instead of two, and implement a straightforward accessor. Command-line option parsing already used to write to a global variable, so it doesn't change much.

New patch serie follows,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Aug 30, 2010, 23:15 UTC · re: Matthieu Moy · lore

[PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

The same pattern is used in many tests, and makes it easy for new ones to rely on $HOME being a trashable, clean, directory.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Just re-ordered the patch to make this one the first.
I took Ævar's suggestion of using $TRASH_DIRECTORY instead of $(pwd).
 t/lib-cvs.sh                    |    3 ---
 t/t0001-init.sh                 |    6 ------
 t/t0003-attributes.sh           |    1 -
 t/t5601-clone.sh                |    2 --
 t/t9130-git-svn-authors-file.sh |    2 --
 t/test-lib.sh                   |    3 +++
 6 files changed, 3 insertions(+), 14 deletions(-)
Show changes to 6 files +3 −14

t/lib-cvs.sh, t/t0001-init.sh, t/t0003-attributes.sh, t/t5601-clone.sh, t/t9130-git-svn-authors-file.sh, t/test-lib.sh

diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 648d161..ad90364 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -3,9 +3,6 @@
 . ./test-lib.sh
 
 unset CVS_SERVER
-# for clean cvsps cache
-HOME=$(pwd)
-export HOME
 
 if ! type cvs >/dev/null 2>&1
 then
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 7c0a698..0543723 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -171,8 +171,6 @@ test_expect_success 'init with init.templatedir set' '
 	mkdir templatedir-source &&
 	echo Content >templatedir-source/file &&
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="${HOME}/.gitconfig" &&
 		git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
 		mkdir templatedir-set &&
@@ -188,8 +186,6 @@ test_expect_success 'init with init.templatedir set' '
 
 test_expect_success 'init --bare/--shared overrides system/global config' '
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.bare false &&
@@ -205,8 +201,6 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 
 test_expect_success 'init honors global core.sharedRepository' '
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.sharedRepository 0666 &&
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index de38c7f..114967a 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -15,7 +15,6 @@ attr_check () {
 
 }
 
-
 test_expect_success 'setup' '
 
 	mkdir -p a/b/d a/c &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8abb71a..8617965 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -163,8 +163,6 @@ test_expect_success 'clone a void' '
 
 test_expect_success 'clone respects global branch.autosetuprebase' '
 	(
-		HOME=$(pwd) &&
-		export HOME &&
 		test_config="$HOME/.gitconfig" &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" branch.autosetuprebase remote &&
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 3c4f319..ec0a106 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -95,8 +95,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
 	(
 		rm -r "$GIT_DIR" &&
 		test x = x"$(git config svn.authorsfile)" &&
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		unset GIT_DIR &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3a3d4c4..8e90f43 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -861,6 +861,9 @@ test_create_repo "$test"
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$test" || exit 1
 
+HOME="$TRASH_DIRECTORY"
+export HOME
+
 this_test=${0##*/}
 this_test=${this_test%%-*}
 for skp in $GIT_SKIP_TESTS
-- 
1.7.2.2.175.ga619d.dirty
Ævar Arnfjörð Bjarmason· Aug 31, 2010, 07:42 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

On Mon, Aug 30, 2010 at 23:15, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
Show 8 quoted lines
> The same pattern is used in many tests, and makes it easy for new ones to
> rely on $HOME being a trashable, clean, directory.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Just re-ordered the patch to make this one the first.
>
> I took Ævar's suggestion of using $TRASH_DIRECTORY instead of $(pwd).
Thanks,
Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason· Sep 1, 2010, 07:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

On Tue, Aug 31, 2010 at 07:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 13 quoted lines
> On Mon, Aug 30, 2010 at 23:15, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>> The same pattern is used in many tests, and makes it easy for new ones to
>> rely on $HOME being a trashable, clean, directory.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>> Just re-ordered the patch to make this one the first.
>>
>> I took Ævar's suggestion of using $TRASH_DIRECTORY instead of $(pwd).
>
> Thanks,
>
> Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Junio: FYI you picked up v1 of this for next/pu, not this v2.
Junio C Hamano· Sep 1, 2010, 15:24 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 16 quoted lines
> On Tue, Aug 31, 2010 at 07:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Mon, Aug 30, 2010 at 23:15, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>> The same pattern is used in many tests, and makes it easy for new ones to
>>> rely on $HOME being a trashable, clean, directory.
>>>
>>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>>> ---
>>> Just re-ordered the patch to make this one the first.
>>>
>>> I took Ævar's suggestion of using $TRASH_DIRECTORY instead of $(pwd).
>>
>> Thanks,
>>
>> Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> Junio: FYI you picked up v1 of this for next/pu, not this v2.
I know.

The originals all used $(pwd) as far as I saw, and it _is_ more faithful and correct refactoring not to use $TRASH_DIRECTORY in this patch, no? You can choose to change it to use $TRASH but that should be done in a separate patch.

Ævar Arnfjörð Bjarmason· Sep 1, 2010, 15:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

On Wed, Sep 1, 2010 at 15:24, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, Aug 31, 2010 at 07:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> On Mon, Aug 30, 2010 at 23:15, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>>> The same pattern is used in many tests, and makes it easy for new ones to
>>>> rely on $HOME being a trashable, clean, directory.
>>>>
>>>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>>>> ---
>>>> Just re-ordered the patch to make this one the first.
>>>>
>>>> I took Ævar's suggestion of using $TRASH_DIRECTORY instead of $(pwd).
>>>
>>> Thanks,
>>>
>>> Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>
>> Junio: FYI you picked up v1 of this for next/pu, not this v2.
>
> I know.
>
> The originals all used $(pwd) as far as I saw, and it _is_ more faithful
> and correct refactoring not to use $TRASH_DIRECTORY in this patch, no?
> You can choose to change it to use $TRASH but that should be done in a
> separate patch.

I just wanted to note it in case you didn't see it. It's a trivial issue and I don't really care, I just wanted to note it in case the v2 went past you.

Matthieu Moy· Sep 1, 2010, 16:57 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 8 quoted lines
>> The originals all used $(pwd) as far as I saw, and it _is_ more faithful
>> and correct refactoring not to use $TRASH_DIRECTORY in this patch, no?
>> You can choose to change it to use $TRASH but that should be done in a
>> separate patch.
>
> I just wanted to note it in case you didn't see it. It's a trivial
> issue and I don't really care, I just wanted to note it in case the v2
> went past you.
Just for the record: I don't care either ;-).
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Aug 30, 2010, 23:15 UTC · re: Matthieu Moy · lore

[PATCH 2/3] don't write to git_log_output_encoding outside git_config()

The log encoding can be given by the user either with --encoding=foo or with i18n.logoutputencoding. The code dealing with this used to write to git_log_output_encoding in both places, making sure that --encoding=foo is dealt with after reading the configuration file.

This is a very fragile mechanism, since any further call to git_config(git_default_config, ...) the value given on the command line.

Instead, keep the config value and the cli value, and decide which one to take at read time (in the straightforward accessor get_git_log_output_encoding()).

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
So, this isn't strictly necessary since the new version of the patch
implementing the gitattributes file doesn't read the full config
anymore, but I think that makes the code more robust.
 builtin/log.c |    4 ++--
 cache.h       |   18 ++++++++++++++++++
 environment.c |    4 +++-
 pretty.c      |    4 ++--
 revision.c    |    4 ++--
 5 files changed, 27 insertions(+), 7 deletions(-)
Show changes to 5 files +27 −7

builtin/log.c, cache.h, environment.c, pretty.c, revision.c

diff --git a/builtin/log.c b/builtin/log.c
index eaa1ee0..f30a6ba 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -329,8 +329,8 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	struct strbuf out = STRBUF_INIT;
 
 	pp_user_info("Tagger", rev->commit_format, &out, buf, rev->date_mode,
-		git_log_output_encoding ?
-		git_log_output_encoding: git_commit_encoding);
+		get_git_log_output_encoding() ?
+		get_git_log_output_encoding(): git_commit_encoding);
 	printf("%s", out.buf);
 	strbuf_release(&out);
 }
diff --git a/cache.h b/cache.h
index eb77e1d..7e10a39 100644
--- a/cache.h
+++ b/cache.h
@@ -1005,7 +1005,25 @@ extern int user_ident_explicitly_given;
 extern int user_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
+
+/* Value found in config file */
 extern const char *git_log_output_encoding;
+
+/* Value given in command line with --encoding */
+extern const char *git_log_output_encoding_cli;
+
+/* 
+ * Prioritize the value given by the command-line over the value found
+ * in the config file.
+ */
+static inline
+const char *get_git_log_output_encoding()
+{
+	return git_log_output_encoding_cli ?
+		git_log_output_encoding_cli :
+		git_log_output_encoding;
+}
+
 extern const char *git_mailmap_file;
 
 /* IO helper functions */
diff --git a/environment.c b/environment.c
index 83d38d3..212f086 100644
--- a/environment.c
+++ b/environment.c
@@ -23,7 +23,9 @@ int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 const char *git_commit_encoding;
-const char *git_log_output_encoding;
+const char *git_log_output_encoding = NULL;
+const char *git_log_output_encoding_cli = NULL;
+
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
 const char *apply_default_ignorewhitespace;
diff --git a/pretty.c b/pretty.c
index f85444b..4187a50 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1159,8 +1159,8 @@ char *reencode_commit_message(const struct commit *commit, const char **encoding
 {
 	const char *encoding;
 
-	encoding = (git_log_output_encoding
-		    ? git_log_output_encoding
+	encoding = (get_git_log_output_encoding()
+		    ? get_git_log_output_encoding()
 		    : git_commit_encoding);
 	if (!encoding)
 		encoding = "UTF-8";
diff --git a/revision.c b/revision.c
index b1c1890..791c757 100644
--- a/revision.c
+++ b/revision.c
@@ -1402,9 +1402,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->grep_filter.all_match = 1;
 	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
 		if (strcmp(optarg, "none"))
-			git_log_output_encoding = xstrdup(optarg);
+			git_log_output_encoding_cli = xstrdup(optarg);
 		else
-			git_log_output_encoding = "";
+			git_log_output_encoding_cli = "";
 		return argcount;
 	} else if (!strcmp(arg, "--reverse")) {
 		revs->reverse ^= 1;
-- 
1.7.2.2.175.ga619d.dirty
Matthieu Moy· Sep 2, 2010, 08:56 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/3] don't write to git_log_output_encoding outside git_config()

Hi,

I'm just raising attention on this patch. It didn't receive any comment and didn't find its way to pu.

I think it makes the code more robust, and would prevent accidental bugs like the one I introduced patching the gitattributes patch, but it's not critical.

Anyway, if you (i.e. the mailing list, not Junio) don't like the patch, I'd prefer to have counter-argument than having the patch silently ignored ;-).

Thanks,
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 116 quoted lines
> The log encoding can be given by the user either with --encoding=foo or
> with i18n.logoutputencoding. The code dealing with this used to write to
> git_log_output_encoding in both places, making sure that --encoding=foo
> is dealt with after reading the configuration file.
>
> This is a very fragile mechanism, since any further call to
> git_config(git_default_config, ...) the value given on the command line.
>
> Instead, keep the config value and the cli value, and decide which one to
> take at read time (in the straightforward accessor
> get_git_log_output_encoding()).
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> So, this isn't strictly necessary since the new version of the patch
> implementing the gitattributes file doesn't read the full config
> anymore, but I think that makes the code more robust.
>
>  builtin/log.c |    4 ++--
>  cache.h       |   18 ++++++++++++++++++
>  environment.c |    4 +++-
>  pretty.c      |    4 ++--
>  revision.c    |    4 ++--
>  5 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index eaa1ee0..f30a6ba 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -329,8 +329,8 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
>  	struct strbuf out = STRBUF_INIT;
>  
>  	pp_user_info("Tagger", rev->commit_format, &out, buf, rev->date_mode,
> -		git_log_output_encoding ?
> -		git_log_output_encoding: git_commit_encoding);
> +		get_git_log_output_encoding() ?
> +		get_git_log_output_encoding(): git_commit_encoding);
>  	printf("%s", out.buf);
>  	strbuf_release(&out);
>  }
> diff --git a/cache.h b/cache.h
> index eb77e1d..7e10a39 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1005,7 +1005,25 @@ extern int user_ident_explicitly_given;
>  extern int user_ident_sufficiently_given(void);
>  
>  extern const char *git_commit_encoding;
> +
> +/* Value found in config file */
>  extern const char *git_log_output_encoding;
> +
> +/* Value given in command line with --encoding */
> +extern const char *git_log_output_encoding_cli;
> +
> +/* 
> + * Prioritize the value given by the command-line over the value found
> + * in the config file.
> + */
> +static inline
> +const char *get_git_log_output_encoding()
> +{
> +	return git_log_output_encoding_cli ?
> +		git_log_output_encoding_cli :
> +		git_log_output_encoding;
> +}
> +
>  extern const char *git_mailmap_file;
>  
>  /* IO helper functions */
> diff --git a/environment.c b/environment.c
> index 83d38d3..212f086 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -23,7 +23,9 @@ int log_all_ref_updates = -1; /* unspecified */
>  int warn_ambiguous_refs = 1;
>  int repository_format_version;
>  const char *git_commit_encoding;
> -const char *git_log_output_encoding;
> +const char *git_log_output_encoding = NULL;
> +const char *git_log_output_encoding_cli = NULL;
> +
>  int shared_repository = PERM_UMASK;
>  const char *apply_default_whitespace;
>  const char *apply_default_ignorewhitespace;
> diff --git a/pretty.c b/pretty.c
> index f85444b..4187a50 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1159,8 +1159,8 @@ char *reencode_commit_message(const struct commit *commit, const char **encoding
>  {
>  	const char *encoding;
>  
> -	encoding = (git_log_output_encoding
> -		    ? git_log_output_encoding
> +	encoding = (get_git_log_output_encoding()
> +		    ? get_git_log_output_encoding()
>  		    : git_commit_encoding);
>  	if (!encoding)
>  		encoding = "UTF-8";
> diff --git a/revision.c b/revision.c
> index b1c1890..791c757 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1402,9 +1402,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->grep_filter.all_match = 1;
>  	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
>  		if (strcmp(optarg, "none"))
> -			git_log_output_encoding = xstrdup(optarg);
> +			git_log_output_encoding_cli = xstrdup(optarg);
>  		else
> -			git_log_output_encoding = "";
> +			git_log_output_encoding_cli = "";
>  		return argcount;
>  	} else if (!strcmp(arg, "--reverse")) {
>  		revs->reverse ^= 1;
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Sep 2, 2010, 15:49 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/3] don't write to git_log_output_encoding outside git_config()

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 6 quoted lines
> I'm just raising attention on this patch. It didn't receive any
> comment and didn't find its way to pu.
>
> I think it makes the code more robust, and would prevent accidental
> bugs like the one I introduced patching the gitattributes patch, but
> it's not critical.

If we didn't have the fix to po/etc-gitattributes topic, the patch is absolutely necessary as it stops bleeding.

Two points and a bit of discussion.
(1) The patch claims that the existing "have a variable in environment.c,
    with possibly hardcoded default, update that variable by reading
    config file at the startup, further override the same variable by
    reading the command line, and use the final result from the variable"
    pattern is fragile, _if_ configuration files can be re-read at random
    places outside the control of the main program.
    The above claim _is_ correct, but is it the existing pattern that is
    broken, or the uncontrolled reading of configuration file, that is the
    real culprit (iow, you said "X if Y" but if Y is false then what X
    claims does not matter)?
    I don't think we have a satisfactory answer to that question yet.
(2) If the answer to the question (1) is that the existing pattern is bad,
    I agree that a mechanism to protect the value that the main program
    decided to use (after taking environment.c default, config and its
    command line processing) _is_ necessary, and the patch shows one
    possible way to do so by replacing a single environment.c variable
    with a pair of variables and an accessor macro.
    (a) Is that the best solution to the problem, though?
    (b) Does the solution show us a pattern that is easy to follow to
        avoid the same problem with other environment.c variables?

One minor potential flaw I see is that while this solution can be applied to a variable of a type with an obvious "unset" value (e.g. pointer with NULL), we cannot use it for a variable of type "int" if it can truly take any value, because get_git_frotz() macro needs to be able to check if git_fortz_cli has that "unset" value in order to decide which one of the variables to use. My gut feeling is that it wouldn't matter in real-life (most int-type knobs actually take uint values), but I didn't check.

I care about (2-b) more than anything else. For example, we see "git_commit_encoding" variable in cache.h in the context of your patch. This particular patch does not have to (and I do not want it to) fix potential problems with that or any other variable, but it would be nice if the patch shows us an obvious and uniform way to apply the same fix when/if it becomes necessary. IOW, we would want to make sure that this is a generic enough solution, not an ad-hoc workaround.

Is there something we can do to make it easier to apply to other cases? Perhaps something along the lines of...

    #define git_declare_var_pair(type, name, unset) \
    extern type name; \
    extern type name ## _cli; \
    static inline type get_ ## name (void) { \
    	return (name ## _cli != (unset)) \
        	? name ## _cli \
                : name; \
    }
    git_declare_var_pair(const char *, git_log_output_encoding, NULL)
Or is it a way-premature generalization?

By the way, do we have a case where the main codepath reads an environment variable and updates a variable in environment.c to be used with that value? Then the existing "fragile" pattern for that case may look like:

    - there is a variable with default value in environment.c;
    - it is overwritten by reading configuration file;
    - it is further overwritten by reading environ[];
    - it is further overwritten by reading argv[];
    - and then it is used.

Does the solution presented by this patch show us a pattern that is applicable to such a case as well?

Show 28 quoted lines
>> diff --git a/cache.h b/cache.h
>> index eb77e1d..7e10a39 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1005,7 +1005,25 @@ extern int user_ident_explicitly_given;
>>  extern int user_ident_sufficiently_given(void);
>>  
>>  extern const char *git_commit_encoding;
>> +
>> +/* Value found in config file */
>>  extern const char *git_log_output_encoding;
>> +
>> +/* Value given in command line with --encoding */
>> +extern const char *git_log_output_encoding_cli;
>> +
>> +/* 
>> + * Prioritize the value given by the command-line over the value found
>> + * in the config file.
>> + */
>> +static inline
>> +const char *get_git_log_output_encoding()
>> +{
>> +	return git_log_output_encoding_cli ?
>> +		git_log_output_encoding_cli :
>> +		git_log_output_encoding;
>> +}
>> +
>>  extern const char *git_mailmap_file;
Matthieu Moy· Aug 30, 2010, 23:15 UTC · re: Matthieu Moy · lore

[PATCH 3/3 v4] Add global and system-wide gitattributes

From: Petr Onderka <gsvick@gmail.com>

Allow gitattributes to be set globally and system wide. This way, settings for particular file types can be set in one place and apply for all user's repositories.

The location of system-wide attributes file is $(prefix)/etc/gitattributes. The location of the global file can be configured by setting core.attributesfile.

Some parts of the code were copied from the implementation of the same functionality in config.c.

Signed-off-by: Petr Onderka <gsvick@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This version doesn't touch config.c, and calls git_config with a
trivial callback reading only the core.attributesfile variable.
This time, I did run the whole testsuite ;-).
 Documentation/config.txt        |    6 ++++
 Documentation/gitattributes.txt |   13 +++++++--
 Makefile                        |    6 ++++
 attr.c                          |   50 ++++++++++++++++++++++++++++++++++++++-
 cache.h                         |    1 +
 configure.ac                    |   10 +++++++-
 environment.c                   |    1 +
 t/t0003-attributes.sh           |   13 ++++++++++
 8 files changed, 95 insertions(+), 5 deletions(-)
Show changes to 8 files +95 −5

Documentation/config.txt, Documentation/gitattributes.txt, Makefile, attr.c, cache.h, configure.ac, environment.c, t/t0003-attributes.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 05ec3fe..0e15e72 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.attributesfile::
+	In addition to '.gitattributes' (per-directory) and
+	'.git/info/attributes', git looks into this file for attributes
+	(see linkgit:gitattributes[5]). Path expansions are made the same
+	way as for `core.excludesfile`.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2e2370c..ebd4852 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -62,14 +62,21 @@ consults `$GIT_DIR/info/attributes` file (which has the highest
 precedence), `.gitattributes` file in the same directory as the
 path in question, and its parent directories up to the toplevel of the
 work tree (the further the directory that contains `.gitattributes`
-is from the path in question, the lower its precedence).
+is from the path in question, the lower its precedence). Finally
+global and system-wide files are considered (they have the lowest
+precedence).
 
 If you wish to affect only a single repository (i.e., to assign
-attributes to files that are particular to one user's workflow), then
+attributes to files that are particular to
+one user's workflow for that repository), then
 attributes should be placed in the `$GIT_DIR/info/attributes` file.
 Attributes which should be version-controlled and distributed to other
 repositories (i.e., attributes of interest to all users) should go into
-`.gitattributes` files.
+`.gitattributes` files. Attributes that should affect all repositories
+for a single user should be placed in a file specified by the
+`core.attributesfile` configuration option (see linkgit:git-config[1]).
+Attributes for all users on a system should be placed in the
+`$(prefix)/etc/gitattributes` file.
 
 Sometimes you would need to override an setting of an attribute
 for a path to `unspecified` state.  This can be done by listing
diff --git a/Makefile b/Makefile
index b4745a5..fdb7b4e 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,7 @@ STRIP ?= strip
 #   infodir
 #   htmldir
 #   ETC_GITCONFIG (but not sysconfdir)
+#   ETC_GITATTRIBUTES
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -286,9 +287,11 @@ htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 else
 sysconfdir = $(prefix)/etc
 ETC_GITCONFIG = etc/gitconfig
+ETC_GITATTRIBUTES = etc/gitattributes
 endif
 lib = lib
 # DESTDIR=
@@ -1502,6 +1505,7 @@ endif
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
@@ -1873,6 +1877,8 @@ builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 
 config.s config.o: EXTRA_CPPFLAGS = -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.s attr.o: EXTRA_CPPFLAGS = -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+
 http.s http.o: EXTRA_CPPFLAGS = -DGIT_HTTP_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
diff --git a/attr.c b/attr.c
index 8ba606c..eeb80d3 100644
--- a/attr.c
+++ b/attr.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "exec_cmd.h"
 #include "attr.h"
 
 const char git_attr__true[] = "(builtin)true";
@@ -462,6 +463,32 @@ static void drop_attr_stack(void)
 	}
 }
 
+const char *git_etc_gitattributes(void)
+{
+	static const char *system_wide;
+	if (!system_wide)
+		system_wide = system_path(ETC_GITATTRIBUTES);
+	return system_wide;
+}
+
+int git_attr_system(void)
+{
+	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
+}
+
+int git_attr_global(void)
+{
+	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
+}
+
+static int git_attr_config(const char *var, const char *value, void *dummy)
+{
+	if (!strcmp(var, "core.attributesfile"))
+		return git_config_pathname(&attributes_file, var, value);
+	
+	return 0;
+}
+
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -472,6 +499,25 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
+		if (git_attr_system()) {
+			elem = read_attr_from_file(git_etc_gitattributes(), 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
+		git_config(git_attr_config, NULL);
+		if (git_attr_global() && attributes_file) {
+			elem = read_attr_from_file(attributes_file, 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
 		if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
 			elem = read_attr(GITATTRIBUTES_FILE, 1);
 			elem->origin = strdup("");
@@ -499,7 +545,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
-	 * set of attribute definitions.  Then, contents from
+	 * set of attribute definitions, followed by the contents
+	 * of $(prefix)/etc/gitattributes and a file specified by
+	 * core.attributesfile.  Then, contents from
 	 * .gitattribute files from directories closer to the
 	 * root to the ones in deeper directories are pushed
 	 * to the stack.  Finally, at the very top of the stack
diff --git a/cache.h b/cache.h
index 7e10a39..4b6e424 100644
--- a/cache.h
+++ b/cache.h
@@ -1051,6 +1051,7 @@ extern int pager_use_color;
 
 extern const char *editor_program;
 extern const char *excludes_file;
+extern const char *attributes_file;
 
 /* base85 */
 int decode_85(char *dst, const char *line, int linelen);
diff --git a/configure.ac b/configure.ac
index 5601e8b..c5b3a41 100644
--- a/configure.ac
+++ b/configure.ac
@@ -282,7 +282,15 @@ GIT_PARSE_WITH(iconv))
 GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
 			Use VALUE instead of /etc/gitconfig as the
 			global git configuration file.
-			If VALUE is not fully qualified it will be interpretted
+			If VALUE is not fully qualified it will be interpreted
+			as a path relative to the computed prefix at runtime.)
+
+#
+# Allow user to set ETC_GITATTRIBUTES variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
+			Use VALUE instead of /etc/gitattributes as the
+			global git attributes file.
+			If VALUE is not fully qualified it will be interpreted
 			as a path relative to the computed prefix at runtime.)
 
 #
diff --git a/environment.c b/environment.c
index 212f086..32c6c96 100644
--- a/environment.c
+++ b/environment.c
@@ -40,6 +40,7 @@ const char *pager_program;
 int pager_use_color = 1;
 const char *editor_program;
 const char *excludes_file;
+const char *attributes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 int read_replace_refs = 1;
 enum eol eol = EOL_UNSET;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 114967a..b884bb7 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -35,6 +35,9 @@ test_expect_success 'setup' '
 		echo "d/* test=a/b/d/*"
 		echo "d/yes notest"
 	) >a/b/.gitattributes
+	(
+		echo "global test=global"
+	) >$HOME/global-gitattributes
 
 '
 
@@ -56,6 +59,16 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_success 'core.attributesfile' '
+	attr_check global unspecified &&
+	git config core.attributesfile "$HOME/global-gitattributes" &&
+	attr_check global global &&
+	git config core.attributesfile "~/global-gitattributes" &&
+	attr_check global global &&
+	echo "global test=precedence" >> .gitattributes &&
+	attr_check global precedence
+'
+
 test_expect_success 'attribute test: read paths from stdin' '
 
 	cat <<EOF > expect
-- 
1.7.2.2.175.ga619d.dirty
Matthieu Moy· Aug 31, 2010, 22:41 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/3 v4] Add global and system-wide gitattributes

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> This version doesn't touch config.c, and calls git_config with a
> trivial callback reading only the core.attributesfile variable.
I see that pu has a variant of my fix:
  17cd572 fixup! Add global and system-wide gitattributes
(is the fixup! here on purpose, or is it a failed rebase -i?)
Junio: your fixup fixes the git_config issue, but doesn't have the
test that my patch have. The difference is that your fixup moves the
global variable attributes_file in attr.c, which sounds like a good
idea. I'm resending my patch with this change for conveinience.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Aug 31, 2010, 22:42 UTC · re: Matthieu Moy · lore

[PATCH] Add global and system-wide gitattributes

From: Petr Onderka <gsvick@gmail.com>

Allow gitattributes to be set globally and system wide. This way, settings for particular file types can be set in one place and apply for all user's repositories.

The location of system-wide attributes file is $(prefix)/etc/gitattributes. The location of the global file can be configured by setting core.attributesfile.

Some parts of the code were copied from the implementation of the same functionality in config.c.

Signed-off-by: Petr Onderka <gsvick@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Here it is!
 Documentation/config.txt        |    6 ++++
 Documentation/gitattributes.txt |   13 +++++++--
 Makefile                        |    6 ++++
 attr.c                          |   52 ++++++++++++++++++++++++++++++++++++++-
 configure.ac                    |   10 ++++++-
 t/t0003-attributes.sh           |   13 +++++++++
 6 files changed, 95 insertions(+), 5 deletions(-)
Show changes to 6 files +95 −5

Documentation/config.txt, Documentation/gitattributes.txt, Makefile, attr.c, configure.ac, t/t0003-attributes.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 05ec3fe..0e15e72 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.attributesfile::
+	In addition to '.gitattributes' (per-directory) and
+	'.git/info/attributes', git looks into this file for attributes
+	(see linkgit:gitattributes[5]). Path expansions are made the same
+	way as for `core.excludesfile`.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2e2370c..ebd4852 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -62,14 +62,21 @@ consults `$GIT_DIR/info/attributes` file (which has the highest
 precedence), `.gitattributes` file in the same directory as the
 path in question, and its parent directories up to the toplevel of the
 work tree (the further the directory that contains `.gitattributes`
-is from the path in question, the lower its precedence).
+is from the path in question, the lower its precedence). Finally
+global and system-wide files are considered (they have the lowest
+precedence).
 
 If you wish to affect only a single repository (i.e., to assign
-attributes to files that are particular to one user's workflow), then
+attributes to files that are particular to
+one user's workflow for that repository), then
 attributes should be placed in the `$GIT_DIR/info/attributes` file.
 Attributes which should be version-controlled and distributed to other
 repositories (i.e., attributes of interest to all users) should go into
-`.gitattributes` files.
+`.gitattributes` files. Attributes that should affect all repositories
+for a single user should be placed in a file specified by the
+`core.attributesfile` configuration option (see linkgit:git-config[1]).
+Attributes for all users on a system should be placed in the
+`$(prefix)/etc/gitattributes` file.
 
 Sometimes you would need to override an setting of an attribute
 for a path to `unspecified` state.  This can be done by listing
diff --git a/Makefile b/Makefile
index b4745a5..fdb7b4e 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,7 @@ STRIP ?= strip
 #   infodir
 #   htmldir
 #   ETC_GITCONFIG (but not sysconfdir)
+#   ETC_GITATTRIBUTES
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -286,9 +287,11 @@ htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 else
 sysconfdir = $(prefix)/etc
 ETC_GITCONFIG = etc/gitconfig
+ETC_GITATTRIBUTES = etc/gitattributes
 endif
 lib = lib
 # DESTDIR=
@@ -1502,6 +1505,7 @@ endif
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
@@ -1873,6 +1877,8 @@ builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 
 config.s config.o: EXTRA_CPPFLAGS = -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.s attr.o: EXTRA_CPPFLAGS = -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+
 http.s http.o: EXTRA_CPPFLAGS = -DGIT_HTTP_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
diff --git a/attr.c b/attr.c
index 8ba606c..c94211a 100644
--- a/attr.c
+++ b/attr.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "exec_cmd.h"
 #include "attr.h"
 
 const char git_attr__true[] = "(builtin)true";
@@ -10,6 +11,8 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define ATTR__UNSET NULL
 #define ATTR__UNKNOWN git_attr__unknown
 
+static const char *attributes_file;
+
 /*
  * The basic design decision here is that we are not going to have
  * insanely large number of attributes.
@@ -462,6 +465,32 @@ static void drop_attr_stack(void)
 	}
 }
 
+const char *git_etc_gitattributes(void)
+{
+	static const char *system_wide;
+	if (!system_wide)
+		system_wide = system_path(ETC_GITATTRIBUTES);
+	return system_wide;
+}
+
+int git_attr_system(void)
+{
+	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
+}
+
+int git_attr_global(void)
+{
+	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
+}
+
+static int git_attr_config(const char *var, const char *value, void *dummy)
+{
+	if (!strcmp(var, "core.attributesfile"))
+		return git_config_pathname(&attributes_file, var, value);
+	
+	return 0;
+}
+
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -472,6 +501,25 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
+		if (git_attr_system()) {
+			elem = read_attr_from_file(git_etc_gitattributes(), 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
+		git_config(git_attr_config, NULL);
+		if (git_attr_global() && attributes_file) {
+			elem = read_attr_from_file(attributes_file, 1);
+			if (elem) {
+				elem->origin = NULL;
+				elem->prev = attr_stack;
+				attr_stack = elem;
+			}
+		}
+
 		if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
 			elem = read_attr(GITATTRIBUTES_FILE, 1);
 			elem->origin = strdup("");
@@ -499,7 +547,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
-	 * set of attribute definitions.  Then, contents from
+	 * set of attribute definitions, followed by the contents
+	 * of $(prefix)/etc/gitattributes and a file specified by
+	 * core.attributesfile.  Then, contents from
 	 * .gitattribute files from directories closer to the
 	 * root to the ones in deeper directories are pushed
 	 * to the stack.  Finally, at the very top of the stack
diff --git a/configure.ac b/configure.ac
index 5601e8b..c5b3a41 100644
--- a/configure.ac
+++ b/configure.ac
@@ -282,7 +282,15 @@ GIT_PARSE_WITH(iconv))
 GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
 			Use VALUE instead of /etc/gitconfig as the
 			global git configuration file.
-			If VALUE is not fully qualified it will be interpretted
+			If VALUE is not fully qualified it will be interpreted
+			as a path relative to the computed prefix at runtime.)
+
+#
+# Allow user to set ETC_GITATTRIBUTES variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitattributes, ETC_GITATTRIBUTES,
+			Use VALUE instead of /etc/gitattributes as the
+			global git attributes file.
+			If VALUE is not fully qualified it will be interpreted
 			as a path relative to the computed prefix at runtime.)
 
 #
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 114967a..b884bb7 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -35,6 +35,9 @@ test_expect_success 'setup' '
 		echo "d/* test=a/b/d/*"
 		echo "d/yes notest"
 	) >a/b/.gitattributes
+	(
+		echo "global test=global"
+	) >$HOME/global-gitattributes
 
 '
 
@@ -56,6 +59,16 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_success 'core.attributesfile' '
+	attr_check global unspecified &&
+	git config core.attributesfile "$HOME/global-gitattributes" &&
+	attr_check global global &&
+	git config core.attributesfile "~/global-gitattributes" &&
+	attr_check global global &&
+	echo "global test=precedence" >> .gitattributes &&
+	attr_check global precedence
+'
+
 test_expect_success 'attribute test: read paths from stdin' '
 
 	cat <<EOF > expect
-- 
1.7.2.2.175.ga619d.dirty
Junio C Hamano· Aug 31, 2010, 23:56 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/3 v4] Add global and system-wide gitattributes

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 10 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> This version doesn't touch config.c, and calls git_config with a
>> trivial callback reading only the core.attributesfile variable.
>
> I see that pu has a variant of my fix:
>
>   17cd572 fixup! Add global and system-wide gitattributes
>
> (is the fixup! here on purpose, or is it a failed rebase -i?)

It wasn't a failed "rebase -i" but was a reminder to myself. I didn't want to squash that in before discussing on the list.

> ... I'm resending my patch with this change for conveinience.

Will take a look; it will have to wait until my git Wednesday this week to be pushed out, though.

Thanks.
Matthieu Moy· Aug 30, 2010, 09:50 UTC · re: Junio C Hamano · lore

[PATCH] tests: factor HOME=$(pwd) in test-lib.sh

The same pattern is used in many tests, and makes it easy for new ones to rely on $HOME being a trashable, clean, directory.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
>> --- a/t/t0003-attributes.sh
>> +++ b/t/t0003-attributes.sh
>> +HOME=$(pwd)
>
> I see a few tests here and there that uses the test-trash directory as
> HOME; perhaps we should have done that in test-lib.sh to make our life
> easier?

I just mimicked what the other tests were doing, but doing it in test-lib.sh makes sense, yes. That also prevents accidental modification of the actual $HOME when writing buggy tests.

The test-suite still passes if I apply this, so it's probably as easy as it seems to be :-). The patch comes on top of mine for .gitattributes, to remove my HOME=$(pwd).

 t/lib-cvs.sh                    |    3 ---
 t/t0001-init.sh                 |    6 ------
 t/t0003-attributes.sh           |    2 --
 t/t5601-clone.sh                |    2 --
 t/t9130-git-svn-authors-file.sh |    2 --
 t/test-lib.sh                   |    3 +++
 6 files changed, 3 insertions(+), 15 deletions(-)
Show changes to 6 files +3 −15

t/lib-cvs.sh, t/t0001-init.sh, t/t0003-attributes.sh, t/t5601-clone.sh, t/t9130-git-svn-authors-file.sh, t/test-lib.sh

diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 648d161..ad90364 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -3,9 +3,6 @@
 . ./test-lib.sh
 
 unset CVS_SERVER
-# for clean cvsps cache
-HOME=$(pwd)
-export HOME
 
 if ! type cvs >/dev/null 2>&1
 then
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 7c0a698..0543723 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -171,8 +171,6 @@ test_expect_success 'init with init.templatedir set' '
 	mkdir templatedir-source &&
 	echo Content >templatedir-source/file &&
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="${HOME}/.gitconfig" &&
 		git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
 		mkdir templatedir-set &&
@@ -188,8 +186,6 @@ test_expect_success 'init with init.templatedir set' '
 
 test_expect_success 'init --bare/--shared overrides system/global config' '
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.bare false &&
@@ -205,8 +201,6 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 
 test_expect_success 'init honors global core.sharedRepository' '
 	(
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.sharedRepository 0666 &&
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 24286e5..b884bb7 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -15,8 +15,6 @@ attr_check () {
 
 }
 
-HOME=$(pwd)
-
 test_expect_success 'setup' '
 
 	mkdir -p a/b/d a/c &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8abb71a..8617965 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -163,8 +163,6 @@ test_expect_success 'clone a void' '
 
 test_expect_success 'clone respects global branch.autosetuprebase' '
 	(
-		HOME=$(pwd) &&
-		export HOME &&
 		test_config="$HOME/.gitconfig" &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" branch.autosetuprebase remote &&
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 3c4f319..ec0a106 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -95,8 +95,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
 	(
 		rm -r "$GIT_DIR" &&
 		test x = x"$(git config svn.authorsfile)" &&
-		HOME="`pwd`" &&
-		export HOME &&
 		test_config="$HOME"/.gitconfig &&
 		unset GIT_CONFIG_NOGLOBAL &&
 		unset GIT_DIR &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3a3d4c4..4eff908 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -861,6 +861,9 @@ test_create_repo "$test"
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$test" || exit 1
 
+HOME=$(pwd)
+export HOME
+
 this_test=${0##*/}
 this_test=${this_test%%-*}
 for skp in $GIT_SKIP_TESTS
-- 
1.7.2.2.175.ga619d.dirty
Ævar Arnfjörð Bjarmason· Aug 30, 2010, 10:22 UTC · re: Matthieu Moy · lore

Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh

On Mon, Aug 30, 2010 at 09:50, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> The same pattern is used in many tests, and makes it easy for new ones to
> rely on $HOME being a trashable, clean, directory.
Looks good, but why not:
> +HOME=$(pwd)
> +export HOME
This instead:

HOME=$TRASH_DIRECTORY export HOME

Looks like it might be more correct given this (always an absolute path), but I haven't tested:

case "$test" in
/*) TRASH_DIRECTORY="$test" ;;
 *) TRASH_DIRECTORY="$TEST_DIRECTORY/$test" ;;
esac
Matthieu Moy· Aug 30, 2010, 10:54 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 16 quoted lines
> On Mon, Aug 30, 2010 at 09:50, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>> The same pattern is used in many tests, and makes it easy for new ones to
>> rely on $HOME being a trashable, clean, directory.
>
> Looks good, but why not:
>
>> +HOME=$(pwd)
>> +export HOME
>
> This instead:
>
> HOME=$TRASH_DIRECTORY
> export HOME
>
> Looks like it might be more correct given this (always an absolute
> path), but I haven't tested:

That should work too, but with your version, I have to think harder to make sure $TRASH_DIRECTORY is absolute (that should be OK), and won't make any issue with symlinks (I have no idea whether it is OK or not), while it's trivially correct with mine (pwd is absolute, and the -P option of the cd command right above prevents issue with symlinks).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ævar Arnfjörð Bjarmason· Aug 30, 2010, 11:08 UTC · re: Matthieu Moy · lore

Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh

On Mon, Aug 30, 2010 at 10:54, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 24 quoted lines
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, Aug 30, 2010 at 09:50, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>> The same pattern is used in many tests, and makes it easy for new ones to
>>> rely on $HOME being a trashable, clean, directory.
>>
>> Looks good, but why not:
>>
>>> +HOME=$(pwd)
>>> +export HOME
>>
>> This instead:
>>
>> HOME=$TRASH_DIRECTORY
>> export HOME
>>
>> Looks like it might be more correct given this (always an absolute
>> path), but I haven't tested:
>
> That should work too, but with your version, I have to think harder to
> make sure $TRASH_DIRECTORY is absolute (that should be OK), and won't
> make any issue with symlinks (I have no idea whether it is OK or not),
> while it's trivially correct with mine (pwd is absolute, and the -P
> option of the cd command right above prevents issue with symlinks).
I don't know what's best here (and I didn't look hard at this).

but I recently brought down the number of $(pwd) invocations in test-lib.sh down to exactly 1, so everything that comes later is now defined in terms of that pwd invocation.

Defining everything that comes afterwards in terms of that pwd might be clearer.

But it's a trivial issue.

← back to recent threads