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

6 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:25

Junio C Hamano [off-list ref] writes:
Junio C Hamano [off-list ref] writes:
quoted
Matthieu Moy [off-list ref] writes:
quoted
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/

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:25

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

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:25

From: Petr Onderka <redacted>

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 <redacted>
Signed-off-by: Matthieu Moy <redacted>
---
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

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:25

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

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

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2016-06-15 22:49:25

On Mon, Aug 30, 2010 at 23:15, Matthieu Moy [off-list ref] 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 <redacted>
---
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 <redacted>

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

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2016-06-15 22:49:27

On Tue, Aug 31, 2010 at 07:42, Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Mon, Aug 30, 2010 at 23:15, Matthieu Moy [off-list ref] wrote:
quoted
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 <redacted>
---
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 <redacted>
Junio: FYI you picked up v1 of this for next/pu, not this v2.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help