[PATCH v2 0/6] git-check-attr should work for relative paths

[PATCH v2 0/6] git-check-attr should work for relative paths

[Michael Haggerty]

This is a reroll of the earlier RFC.  It must be applied on top of
"Add --all option to git-check-attr" v3.

This re-roll differs in the following ways from the RFC:

* Adjusted for changes between the "--all" patch series v2 -> v3.

* Fixed some C problems found by Junio.


Currently, git-check-attr gets confused if it is passed certain types
of unnormalized paths or is invoked from a non-top-level directory.
For example, using the test repo created by t0003-attributes.sh,
inquiring about a path that starts with "./" causes git-check-attr to
emit a spurious warning:

    $ git check-attr test ./f
    [attr]notest !test
     not allowed: ./.gitattributes:1
    ./f: test: f
    $ git check-attr test ./a/i
    [attr]notest !test
     not allowed: ./.gitattributes:1
    ./a/i: test: a/i

In these cases it seems to find the right values, even including the
application of macros from the top-level .gitattributes file.  (My
guess is that it is loading the top-level .gitattributes file
twice--once as a top-level file, and once believing that it is in a
subdirectory.)

Invoking git-check-attr from a subdirectory using a relative path
gives incorrect answers; for example:

    $ (cd a; git check-attr test i )
    i: test: unspecified

(On the other hand, if invoked using the full path of a file, it
"works":

    $ (cd a; git check-attr test a/i )
    a/i: test: a/i

.)  I think that this behavior is confusing and inconsistent with
other git commands.  It is also inconsistent with the behavior of
.gitignore, which works from subdirectories.

The patches in this series add tests demonstrating the problem,
propose a possible solution, and add a couple of subcommands to
test-path-utils.

The changes to test-path-utils helped me figure out how the
corresponding functions work (comments? we don't need no stinkin'
comments) but they are not used anywhere.  Take 'em or leave 'em.


Michael Haggerty (6):
  git-check-attr: test that no output is written to stderr
  git-check-attr: Demonstrate problems with unnormalized paths
  git-check-attr: Demonstrate problems with relative paths
  git-check-attr: Normalize paths
  test-path-utils: Add subcommand "absolute_path"
  test-path-utils: Add subcommand "prefix_path"

 builtin/check-attr.c  |   20 ++++++++++++--------
 t/t0003-attributes.sh |   29 ++++++++++++++++++++++++++---
 test-path-utils.c     |   22 ++++++++++++++++++++++
 3 files changed, 60 insertions(+), 11 deletions(-)

-- 
1.7.6.8.gd2879

[PATCH v2 1/6] git-check-attr: test that no output is written to stderr

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0003-attributes.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5acb2d5..4f2fbc0 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -9,9 +9,10 @@ attr_check () {
 	path="$1"
 	expect="$2"
 
-	git check-attr test -- "$path" >actual &&
+	git check-attr test -- "$path" >actual 2>err &&
 	echo "$path: test: $2" >expect &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_line_count = 0 err
 
 }
 
-- 
1.7.6.8.gd2879

[PATCH v2 2/6] git-check-attr: Demonstrate problems with unnormalized paths

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0003-attributes.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 4f2fbc0..43957ea 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -93,6 +93,15 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_failure 'unnormalized paths' '
+
+	attr_check ./f f &&
+	attr_check ./a/g a/g &&
+	attr_check a/./g a/g &&
+	attr_check a/c/../b/g a/b/g
+
+'
+
 test_expect_success 'core.attributesfile' '
 	attr_check global unspecified &&
 	git config core.attributesfile "$HOME/global-gitattributes" &&
-- 
1.7.6.8.gd2879

[PATCH v2 3/6] git-check-attr: Demonstrate problems with relative paths

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0003-attributes.sh |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 43957ea..f6cf77d 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -19,7 +19,7 @@ attr_check () {
 
 test_expect_success 'setup' '
 
-	mkdir -p a/b/d a/c &&
+	mkdir -p a/b/d a/c b &&
 	(
 		echo "[attr]notest !test"
 		echo "f	test=f"
@@ -102,6 +102,19 @@ test_expect_failure 'unnormalized paths' '
 
 '
 
+test_expect_failure 'relative paths' '
+
+	(cd a && attr_check ../f f) &&
+	(cd a && attr_check f f) &&
+	(cd a && attr_check i a/i) &&
+	(cd a && attr_check g a/g) &&
+	(cd a && attr_check b/g a/b/g) &&
+	(cd b && attr_check ../a/f f) &&
+	(cd b && attr_check ../a/g a/g) &&
+	(cd b && attr_check ../a/b/g a/b/g)
+
+'
+
 test_expect_success 'core.attributesfile' '
 	attr_check global unspecified &&
 	git config core.attributesfile "$HOME/global-gitattributes" &&
-- 
1.7.6.8.gd2879

[PATCH v2 4/6] git-check-attr: Normalize paths

[Michael Haggerty]

Normalize the path arguments (relative to the working tree root, if
applicable) before looking up their attributes.  This requires passing
the prefix down the call chain.

This fixes two test cases for different reasons:

* "unnormalized paths" is fixed because the .gitattribute-file-seeking
  code is not confused into reading the top-level file twice.

* "relative paths" is fixed because the canonical pathnames are passed
  to get_check_attr() or get_all_attrs(), allowing them to match the
  pathname patterns as expected.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/check-attr.c  |   20 ++++++++++++--------
 t/t0003-attributes.sh |    4 ++--
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 6b16368..708988a 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -41,22 +41,26 @@ static void output_attr(int cnt, struct git_attr_check *check,
 	}
 }
 
-static void check_attr(int cnt, struct git_attr_check *check,
-	const char *file)
+static void check_attr(const char *prefix, int cnt,
+	struct git_attr_check *check, const char *file)
 {
+	char *full_path =
+		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
 	if (check != NULL) {
-		if (git_check_attr(file, cnt, check))
+		if (git_check_attr(full_path, cnt, check))
 			die("git_check_attr died");
 		output_attr(cnt, check, file);
 	} else {
-		if (git_all_attrs(file, &cnt, &check))
+		if (git_all_attrs(full_path, &cnt, &check))
 			die("git_all_attrs died");
 		output_attr(cnt, check, file);
 		free(check);
 	}
+	free(full_path);
 }
 
-static void check_attr_stdin_paths(int cnt, struct git_attr_check *check)
+static void check_attr_stdin_paths(const char *prefix, int cnt,
+	struct git_attr_check *check)
 {
 	struct strbuf buf, nbuf;
 	int line_termination = null_term_line ? 0 : '\n';
@@ -70,7 +74,7 @@ static void check_attr_stdin_paths(int cnt, struct git_attr_check *check)
 				die("line is badly quoted");
 			strbuf_swap(&buf, &nbuf);
 		}
-		check_attr(cnt, check, buf.buf);
+		check_attr(prefix, cnt, check, buf.buf);
 		maybe_flush_or_die(stdout, "attribute to stdout");
 	}
 	strbuf_release(&buf);
@@ -154,10 +158,10 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	}
 
 	if (stdin_paths)
-		check_attr_stdin_paths(cnt, check);
+		check_attr_stdin_paths(prefix, cnt, check);
 	else {
 		for (i = filei; i < argc; i++)
-			check_attr(cnt, check, argv[i]);
+			check_attr(prefix, cnt, check, argv[i]);
 		maybe_flush_or_die(stdout, "attribute to stdout");
 	}
 	return 0;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f6cf77d..ae2f1da 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -93,7 +93,7 @@ test_expect_success 'attribute test' '
 
 '
 
-test_expect_failure 'unnormalized paths' '
+test_expect_success 'unnormalized paths' '
 
 	attr_check ./f f &&
 	attr_check ./a/g a/g &&
@@ -102,7 +102,7 @@ test_expect_failure 'unnormalized paths' '
 
 '
 
-test_expect_failure 'relative paths' '
+test_expect_success 'relative paths' '
 
 	(cd a && attr_check ../f f) &&
 	(cd a && attr_check f f) &&
-- 
1.7.6.8.gd2879

[PATCH v2 5/6] test-path-utils: Add subcommand "absolute_path"

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 test-path-utils.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/test-path-utils.c b/test-path-utils.c
index e767159..a410e30 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -20,6 +20,15 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
+	if (argc >= 2 && !strcmp(argv[1], "absolute_path")) {
+		while (argc > 2) {
+			puts(absolute_path(argv[2]));
+			argc--;
+			argv++;
+		}
+		return 0;
+	}
+
 	if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
 		int len = longest_ancestor_length(argv[2], argv[3]);
 		printf("%d\n", len);
-- 
1.7.6.8.gd2879

[PATCH v2 6/6] test-path-utils: Add subcommand "prefix_path"

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 test-path-utils.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/test-path-utils.c b/test-path-utils.c
index a410e30..3bc20e9 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -35,6 +35,19 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
+	if (argc >= 4 && !strcmp(argv[1], "prefix_path")) {
+		char *prefix = argv[2];
+		int prefix_len = strlen(prefix);
+		int nongit_ok;
+		setup_git_directory_gently(&nongit_ok);
+		while (argc > 3) {
+			puts(prefix_path(prefix, prefix_len, argv[3]));
+			argc--;
+			argv++;
+		}
+		return 0;
+	}
+
 	if (argc == 4 && !strcmp(argv[1], "strip_path_suffix")) {
 		char *prefix = strip_path_suffix(argv[2], argv[3]);
 		printf("%s\n", prefix ? prefix : "(null)");
-- 
1.7.6.8.gd2879