All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH/RFC] Add global and system-wide gitattributes
@ 2010-08-11  1:04 Petr Onderka
  2010-08-11  9:20 ` Henrik Grubbström
                   ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Petr Onderka @ 2010-08-11  1:04 UTC (permalink / raw)
  To: git; +Cc: Henrik Grubbstrm

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH/RFC] Add global and system-wide gitattributes
  2010-08-11  1:04 [PATCH/RFC] Add global and system-wide gitattributes Petr Onderka
@ 2010-08-11  9:20 ` Henrik Grubbström
  2010-08-11 10:50   ` Petr Onderka
  2010-08-11 12:31 ` Matthieu Moy
  2010-08-11 22:19 ` Junio C Hamano
  2 siblings, 1 reply; 36+ messages in thread
From: Henrik Grubbström @ 2010-08-11  9:20 UTC (permalink / raw)
  To: Petr Onderka; +Cc: git

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2418 bytes --]

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

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

> 			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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH/RFC] Add global and system-wide gitattributes
  2010-08-11  9:20 ` Henrik Grubbström
@ 2010-08-11 10:50   ` Petr Onderka
  0 siblings, 0 replies; 36+ messages in thread
From: Petr Onderka @ 2010-08-11 10:50 UTC (permalink / raw)
  To: Henrik Grubbström; +Cc: git

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH/RFC] Add global and system-wide gitattributes
  2010-08-11  1:04 [PATCH/RFC] Add global and system-wide gitattributes Petr Onderka
  2010-08-11  9:20 ` Henrik Grubbström
@ 2010-08-11 12:31 ` Matthieu Moy
  2010-08-11 22:19 ` Junio C Hamano
  2 siblings, 0 replies; 36+ messages in thread
From: Matthieu Moy @ 2010-08-11 12:31 UTC (permalink / raw)
  To: Petr Onderka; +Cc: git, Henrik Grubbstrm

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/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH/RFC] Add global and system-wide gitattributes
  2010-08-11  1:04 [PATCH/RFC] Add global and system-wide gitattributes Petr Onderka
  2010-08-11  9:20 ` Henrik Grubbström
  2010-08-11 12:31 ` Matthieu Moy
@ 2010-08-11 22:19 ` Junio C Hamano
  2010-08-16 16:51   ` Petr Onderka
  2 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2010-08-11 22:19 UTC (permalink / raw)
  To: Petr Onderka; +Cc: git, Henrik Grubbstrm

Petr Onderka <gsvick@gmail.com> writes:

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH/RFC] Add global and system-wide gitattributes
  2010-08-11 22:19 ` Junio C Hamano
@ 2010-08-16 16:51   ` Petr Onderka
  2010-08-16 16:56     ` [PATCH v2] " Petr Onderka
  0 siblings, 1 reply; 36+ messages in thread
From: Petr Onderka @ 2010-08-16 16:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Henrik Grubbstrm

Hi,

On Thu, Aug 12, 2010 at 00:19, Junio C Hamano <gitster@pobox.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2] Add global and system-wide gitattributes
  2010-08-16 16:51   ` Petr Onderka
@ 2010-08-16 16:56     ` Petr Onderka
  2010-08-25  9:55       ` Štěpán Němec
  2010-08-28 18:35       ` Matthieu Moy
  0 siblings, 2 replies; 36+ messages in thread
From: Petr Onderka @ 2010-08-16 16:56 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Henrik Grubbstrm, Matthieu Moy, Petr Onderka

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2] Add global and system-wide gitattributes
  2010-08-16 16:56     ` [PATCH v2] " Petr Onderka
@ 2010-08-25  9:55       ` Štěpán Němec
  2010-08-28 17:33         ` Matthieu Moy
  2010-08-28 18:35       ` Matthieu Moy
  1 sibling, 1 reply; 36+ messages in thread
From: Štěpán Němec @ 2010-08-25  9:55 UTC (permalink / raw)
  To: Petr Onderka; +Cc: git, Junio C Hamano, Henrik Grubbstrm, Matthieu Moy

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2] Add global and system-wide gitattributes
  2010-08-25  9:55       ` Štěpán Němec
@ 2010-08-28 17:33         ` Matthieu Moy
  2010-08-30  5:34           ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-28 17:33 UTC (permalink / raw)
  To: Štěpán Němec
  Cc: Petr Onderka, git, Junio C Hamano, Henrik Grubbstrm

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

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/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2] Add global and system-wide gitattributes
  2010-08-16 16:56     ` [PATCH v2] " Petr Onderka
  2010-08-25  9:55       ` Štěpán Němec
@ 2010-08-28 18:35       ` Matthieu Moy
  2010-08-28 18:41         ` [PATCH] core.attributesfile: a fix, a simplification, and a test Matthieu Moy
  1 sibling, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-28 18:35 UTC (permalink / raw)
  To: Petr Onderka; +Cc: git, Junio C Hamano, Henrik Grubbstrm

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] core.attributesfile: a fix, a simplification, and a test
  2010-08-28 18:35       ` Matthieu Moy
@ 2010-08-28 18:41         ` Matthieu Moy
  2010-08-29 10:32           ` [PATCH v3?] Add global and system-wide gitattributes Štěpán Němec
  0 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-28 18:41 UTC (permalink / raw)
  To: git; +Cc: Petr Onderka, Matthieu Moy

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-28 18:41         ` [PATCH] core.attributesfile: a fix, a simplification, and a test Matthieu Moy
@ 2010-08-29 10:32           ` Štěpán Němec
  2010-08-30  8:04             ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Štěpán Němec @ 2010-08-29 10:32 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, Petr Onderka, Junio C. Hamano


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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2] Add global and system-wide gitattributes
  2010-08-28 17:33         ` Matthieu Moy
@ 2010-08-30  5:34           ` Junio C Hamano
  2010-08-30  9:09             ` Štěpán Němec
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2010-08-30  5:34 UTC (permalink / raw)
  To: Matthieu Moy
  Cc: Štěpán Němec, Petr Onderka, git, Henrik Grubbstrm

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-29 10:32           ` [PATCH v3?] Add global and system-wide gitattributes Štěpán Němec
@ 2010-08-30  8:04             ` Junio C Hamano
  2010-08-30  8:26               ` Matthieu Moy
  2010-08-30  9:50               ` [PATCH] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
  0 siblings, 2 replies; 36+ messages in thread
From: Junio C Hamano @ 2010-08-30  8:04 UTC (permalink / raw)
  To: Štěpán Němec
  Cc: Matthieu Moy, git, Petr Onderka, Junio C. Hamano

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-30  8:04             ` Junio C Hamano
@ 2010-08-30  8:26               ` Matthieu Moy
  2010-08-30 20:47                 ` Junio C Hamano
  2010-08-30  9:50               ` [PATCH] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
  1 sibling, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30  8:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Štěpán Němec, git, Petr Onderka

Junio C Hamano <gitster@pobox.com> writes:

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

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/

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2] Add global and system-wide gitattributes
  2010-08-30  5:34           ` Junio C Hamano
@ 2010-08-30  9:09             ` Štěpán Němec
  0 siblings, 0 replies; 36+ messages in thread
From: Štěpán Němec @ 2010-08-30  9:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthieu Moy, Petr Onderka, git, Henrik Grubbstrm

Junio C Hamano <gitster@pobox.com> writes:

> 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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30  8:04             ` Junio C Hamano
  2010-08-30  8:26               ` Matthieu Moy
@ 2010-08-30  9:50               ` Matthieu Moy
  2010-08-30 10:22                 ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30  9:50 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

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:

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30  9:50               ` [PATCH] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
@ 2010-08-30 10:22                 ` Ævar Arnfjörð Bjarmason
  2010-08-30 10:54                   ` Matthieu Moy
  0 siblings, 1 reply; 36+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-30 10:22 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30 10:22                 ` Ævar Arnfjörð Bjarmason
@ 2010-08-30 10:54                   ` Matthieu Moy
  2010-08-30 11:08                     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30 10:54 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, gitster

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30 10:54                   ` Matthieu Moy
@ 2010-08-30 11:08                     ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 36+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-30 11:08 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

On Mon, Aug 30, 2010 at 10:54, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ę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.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-30  8:26               ` Matthieu Moy
@ 2010-08-30 20:47                 ` Junio C Hamano
  2010-08-30 21:11                   ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2010-08-30 20:47 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Štěpán Němec, git, Petr Onderka

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?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-30 20:47                 ` Junio C Hamano
@ 2010-08-30 21:11                   ` Junio C Hamano
  2010-08-30 22:55                     ` Matthieu Moy
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2010-08-30 21:11 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Štěpán Němec, git, Petr Onderka

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3?] Add global and system-wide gitattributes
  2010-08-30 21:11                   ` Junio C Hamano
@ 2010-08-30 22:55                     ` Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
                                         ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30 22:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Štěpán Němec, git, Petr Onderka

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30 22:55                     ` Matthieu Moy
@ 2010-08-30 23:15                       ` Matthieu Moy
  2010-08-31  7:42                         ` Ævar Arnfjörð Bjarmason
  2010-08-30 23:15                       ` [PATCH 2/3] don't write to git_log_output_encoding outside git_config() Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 3/3 v4] Add global and system-wide gitattributes Matthieu Moy
  2 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30 23:15 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/3] don't write to git_log_output_encoding outside git_config()
  2010-08-30 22:55                     ` Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
@ 2010-08-30 23:15                       ` Matthieu Moy
  2010-09-02  8:56                         ` Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 3/3 v4] Add global and system-wide gitattributes Matthieu Moy
  2 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30 23:15 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

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;
-- 
1.7.2.2.175.ga619d.dirty

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 3/3 v4] Add global and system-wide gitattributes
  2010-08-30 22:55                     ` Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
  2010-08-30 23:15                       ` [PATCH 2/3] don't write to git_log_output_encoding outside git_config() Matthieu Moy
@ 2010-08-30 23:15                       ` Matthieu Moy
  2010-08-31 22:41                         ` Matthieu Moy
  2 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-08-30 23:15 UTC (permalink / raw)
  To: git, gitster; +Cc: Petr Onderka, Matthieu Moy

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-30 23:15                       ` [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
@ 2010-08-31  7:42                         ` Ævar Arnfjörð Bjarmason
  2010-09-01  7:56                           ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 36+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-31  7:42 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

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>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/3 v4] Add global and system-wide gitattributes
  2010-08-30 23:15                       ` [PATCH 3/3 v4] Add global and system-wide gitattributes Matthieu Moy
@ 2010-08-31 22:41                         ` Matthieu Moy
  2010-08-31 22:42                           ` [PATCH] " Matthieu Moy
  2010-08-31 23:56                           ` [PATCH 3/3 v4] " Junio C Hamano
  0 siblings, 2 replies; 36+ messages in thread
From: Matthieu Moy @ 2010-08-31 22:41 UTC (permalink / raw)
  To: git; +Cc: gitster, Petr Onderka

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/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] Add global and system-wide gitattributes
  2010-08-31 22:41                         ` Matthieu Moy
@ 2010-08-31 22:42                           ` Matthieu Moy
  2010-08-31 23:56                           ` [PATCH 3/3 v4] " Junio C Hamano
  1 sibling, 0 replies; 36+ messages in thread
From: Matthieu Moy @ 2010-08-31 22:42 UTC (permalink / raw)
  To: git, gitster; +Cc: gsvick, Matthieu Moy

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

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/3 v4] Add global and system-wide gitattributes
  2010-08-31 22:41                         ` Matthieu Moy
  2010-08-31 22:42                           ` [PATCH] " Matthieu Moy
@ 2010-08-31 23:56                           ` Junio C Hamano
  1 sibling, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2010-08-31 23:56 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, Petr Onderka

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-08-31  7:42                         ` Ævar Arnfjörð Bjarmason
@ 2010-09-01  7:56                           ` Ævar Arnfjörð Bjarmason
  2010-09-01 15:24                             ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-09-01  7:56 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

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.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-09-01  7:56                           ` Ævar Arnfjörð Bjarmason
@ 2010-09-01 15:24                             ` Junio C Hamano
  2010-09-01 15:40                               ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2010-09-01 15:24 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Matthieu Moy, git, gitster

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-09-01 15:24                             ` Junio C Hamano
@ 2010-09-01 15:40                               ` Ævar Arnfjörð Bjarmason
  2010-09-01 16:57                                 ` Matthieu Moy
  0 siblings, 1 reply; 36+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-09-01 15:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthieu Moy, git

On Wed, Sep 1, 2010 at 15:24, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh
  2010-09-01 15:40                               ` Ævar Arnfjörð Bjarmason
@ 2010-09-01 16:57                                 ` Matthieu Moy
  0 siblings, 0 replies; 36+ messages in thread
From: Matthieu Moy @ 2010-09-01 16:57 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/3] don't write to git_log_output_encoding outside git_config()
  2010-08-30 23:15                       ` [PATCH 2/3] don't write to git_log_output_encoding outside git_config() Matthieu Moy
@ 2010-09-02  8:56                         ` Matthieu Moy
  2010-09-02 15:49                           ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Matthieu Moy @ 2010-09-02  8:56 UTC (permalink / raw)
  To: git; +Cc: gitster

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:

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/3] don't write to git_log_output_encoding outside git_config()
  2010-09-02  8:56                         ` Matthieu Moy
@ 2010-09-02 15:49                           ` Junio C Hamano
  0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2010-09-02 15:49 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2010-09-02 15:52 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-11  1:04 [PATCH/RFC] Add global and system-wide gitattributes Petr Onderka
2010-08-11  9:20 ` Henrik Grubbström
2010-08-11 10:50   ` Petr Onderka
2010-08-11 12:31 ` Matthieu Moy
2010-08-11 22:19 ` Junio C Hamano
2010-08-16 16:51   ` Petr Onderka
2010-08-16 16:56     ` [PATCH v2] " Petr Onderka
2010-08-25  9:55       ` Štěpán Němec
2010-08-28 17:33         ` Matthieu Moy
2010-08-30  5:34           ` Junio C Hamano
2010-08-30  9:09             ` Štěpán Němec
2010-08-28 18:35       ` Matthieu Moy
2010-08-28 18:41         ` [PATCH] core.attributesfile: a fix, a simplification, and a test Matthieu Moy
2010-08-29 10:32           ` [PATCH v3?] Add global and system-wide gitattributes Štěpán Němec
2010-08-30  8:04             ` Junio C Hamano
2010-08-30  8:26               ` Matthieu Moy
2010-08-30 20:47                 ` Junio C Hamano
2010-08-30 21:11                   ` Junio C Hamano
2010-08-30 22:55                     ` Matthieu Moy
2010-08-30 23:15                       ` [PATCH 1/3 v2] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
2010-08-31  7:42                         ` Ævar Arnfjörð Bjarmason
2010-09-01  7:56                           ` Ævar Arnfjörð Bjarmason
2010-09-01 15:24                             ` Junio C Hamano
2010-09-01 15:40                               ` Ævar Arnfjörð Bjarmason
2010-09-01 16:57                                 ` Matthieu Moy
2010-08-30 23:15                       ` [PATCH 2/3] don't write to git_log_output_encoding outside git_config() Matthieu Moy
2010-09-02  8:56                         ` Matthieu Moy
2010-09-02 15:49                           ` Junio C Hamano
2010-08-30 23:15                       ` [PATCH 3/3 v4] Add global and system-wide gitattributes Matthieu Moy
2010-08-31 22:41                         ` Matthieu Moy
2010-08-31 22:42                           ` [PATCH] " Matthieu Moy
2010-08-31 23:56                           ` [PATCH 3/3 v4] " Junio C Hamano
2010-08-30  9:50               ` [PATCH] tests: factor HOME=$(pwd) in test-lib.sh Matthieu Moy
2010-08-30 10:22                 ` Ævar Arnfjörð Bjarmason
2010-08-30 10:54                   ` Matthieu Moy
2010-08-30 11:08                     ` Ævar Arnfjörð Bjarmason

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.