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

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

[Michael Haggerty]

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.

I am far from confident that the solution proposed in patch 4 is
correct; please see the notes in that email for discussion.

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

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

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

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

[RFC 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_checkattr() or get_allattrs(), allowing them to match the
  pathname patterns as expected.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I had a very hard time figuring out the correct approach to fixing
this problem.  There is so little documentation in the C code!  The
suggested patch fixes the tests, but it might be wrong for other
reasons:

1. I'm not sure whether it is correct to fix this problem at the level
   of git-check-attr, or whether the fix belongs in the API layer.
   What is the convention for API functions?  Do they typically take
   path names relative to the CWD or relative to the working tree
   root, or ...?  I also found examples of commands that change the
   current directory during their operation, making the CWD and the
   working tree root identical.  What is preferred?  (And where is it
   documented?)

2. It could be that I'm abusing the prefix_path() function, or that
   there is a better way to achieve the normalization.

 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 781b7df..23a6e07 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -41,22 +41,26 @@ static void output_attr(int cnt, struct git_attr_value *check,
 	}
 }
 
-static void check_attr(int cnt, struct git_attr_value *check,
-	const char *file)
+static void check_attr(const char *prefix, int cnt,
+	struct git_attr_value *check, const char *file)
 {
+	const char *full_path =
+		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
 	if (check != NULL) {
-		if (git_checkattr(file, cnt, check))
+		if (git_checkattr(full_path, cnt, check))
 			die("git_checkattr died");
 		output_attr(cnt, check, file);
 	} else {
-		if (git_allattrs(file, &cnt, &check))
+		if (git_allattrs(full_path, &cnt, &check))
 			die("git_allattrs died");
 		output_attr(cnt, check, file);
 		free(check);
 	}
+	free(full_path);
 }
 
-static void check_attr_stdin_paths(int cnt, struct git_attr_value *check)
+static void check_attr_stdin_paths(const char *prefix, int cnt,
+	struct git_attr_value *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_value *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

Re: [RFC 4/6] git-check-attr: Normalize paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 1. I'm not sure whether it is correct to fix this problem at the level
>    of git-check-attr, or whether the fix belongs in the API layer.
>    What is the convention for API functions?  Do they typically take
>    path names relative to the CWD or relative to the working tree
>    root, or ...?

I think passing down "prefix" (i.e. where your $(cwd) was relative to the
root level of the working tree) and the user-supplied "pathspec" (which
typically is relative to that original $(cwd)) is the canonical approach.
The very original git worked only at the root level of the working tree,
with paths specified relative to the root level of the tree, so many code
do:

	- find out the root of the working tree;
        - note where the $(cwd) was in "prefix";
        - chdir to the root of the working tree;
	- prepend the "prefix" to user supplied pathspec;
        - forget all the complexity and work on the whole tree.

Then the "prefix" gets stripped away from the beginning of the paths when
reporting.

Your patch from a cursory look seems to follow that pattern, which is
good.

Re: [RFC 4/6] git-check-attr: Normalize paths

[Michael Haggerty]

On 08/02/2011 07:24 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 1. I'm not sure whether it is correct to fix this problem at the level
>>    of git-check-attr, or whether the fix belongs in the API layer.
>>    What is the convention for API functions?  Do they typically take
>>    path names relative to the CWD or relative to the working tree
>>    root, or ...?
> 
> I think passing down "prefix" (i.e. where your $(cwd) was relative to the
> root level of the working tree) and the user-supplied "pathspec" (which
> typically is relative to that original $(cwd)) is the canonical approach.
> The very original git worked only at the root level of the working tree,
> with paths specified relative to the root level of the tree, so many code
> do:
> 
> 	- find out the root of the working tree;
>         - note where the $(cwd) was in "prefix";
>         - chdir to the root of the working tree;
> 	- prepend the "prefix" to user supplied pathspec;
>         - forget all the complexity and work on the whole tree.
> 
> Then the "prefix" gets stripped away from the beginning of the paths when
> reporting.
> 
> Your patch from a cursory look seems to follow that pattern, which is
> good.

Thanks for the explanation.

Yes, my code follows the pattern, except that in this case it is
unnecessary to chdir to the root of the working tree.

All this chdiring is going to be a nightmare for the libification of
git, since the cwd is a program-wide global variable with implicit
side-effects on almost any code that deals with the filesystem.  It is
obviously not permissible for a library to change directories (not even
temporarily, if the library is intended to be used in a multithreaded
application).  But that is a topic for another day.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC 4/6] git-check-attr: Normalize paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> I think passing down "prefix" (i.e. where your $(cwd) was relative to the
>> root level of the working tree) and the user-supplied "pathspec" (which
>> typically is relative to that original $(cwd)) is the canonical approach.
>> The very original git worked only at the root level of the working tree,
>> with paths specified relative to the root level of the tree, so many code
>> do:
>> 
>> 	- find out the root of the working tree;
>>         - note where the $(cwd) was in "prefix";
>>         - chdir to the root of the working tree;
>> 	- prepend the "prefix" to user supplied pathspec;
>>         - forget all the complexity and work on the whole tree.
>> 
>> Then the "prefix" gets stripped away from the beginning of the paths when
>> reporting.
>> 
>> Your patch from a cursory look seems to follow that pattern, which is
>> good.
>
> Thanks for the explanation.
>
> Yes, my code follows the pattern, except that in this case it is
> unnecessary to chdir to the root of the working tree.

Just to make sure there is no misunderstanding. The chdir() should not be
in the core part of the system that you may want to libify.

The above pattern was developed primarily so that older utility functions
in the system that were written back when nobody ran git from anywhere
other than the top level of the working tree can be easily adapted to main
programs that can be launched from a subdirectory. The initial set-up part
of the program is responsible for figuring out "prefix", turning relative
paths given by the user into paths relative to the top of the working
tree, and then chdir'ing to the top.

After all that happens, the library-ish parts of the system only have to
deal with full paths relative to the root of the working tree. "prefix"
comes into play when reporting the results (i.e. showing paths relative to
user's $(cwd) in the output or in the error messages).

Re: [RFC 4/6] git-check-attr: Normalize paths

[Michael Haggerty]

On 08/04/2011 07:05 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> I think passing down "prefix" (i.e. where your $(cwd) was relative to the
>>> root level of the working tree) and the user-supplied "pathspec" (which
>>> typically is relative to that original $(cwd)) is the canonical approach.
>>> The very original git worked only at the root level of the working tree,
>>> with paths specified relative to the root level of the tree, so many code
>>> do:
>>>
>>> 	- find out the root of the working tree;
>>>         - note where the $(cwd) was in "prefix";
>>>         - chdir to the root of the working tree;
>>> 	- prepend the "prefix" to user supplied pathspec;
>>>         - forget all the complexity and work on the whole tree.
>>>
>>> Then the "prefix" gets stripped away from the beginning of the paths when
>>> reporting.
>>>
>>> Your patch from a cursory look seems to follow that pattern, which is
>>> good.
>>
>> Thanks for the explanation.
>>
>> Yes, my code follows the pattern, except that in this case it is
>> unnecessary to chdir to the root of the working tree.
> 
> Just to make sure there is no misunderstanding. The chdir() should not be
> in the core part of the system that you may want to libify.
> 
> The above pattern was developed primarily so that older utility functions
> in the system that were written back when nobody ran git from anywhere
> other than the top level of the working tree can be easily adapted to main
> programs that can be launched from a subdirectory. The initial set-up part
> of the program is responsible for figuring out "prefix", turning relative
> paths given by the user into paths relative to the top of the working
> tree, and then chdir'ing to the top.
> 
> After all that happens, the library-ish parts of the system only have to
> deal with full paths relative to the root of the working tree. "prefix"
> comes into play when reporting the results (i.e. showing paths relative to
> user's $(cwd) in the output or in the error messages).

If I understand you correctly, the use of some API routines requires a
chdir by the caller (i.e., the surrounding application) *before* calling
into the routine.  This is certainly a bit cleaner than the library
chdiring itself, but it is still unusable in a multithreaded context.
Regardless of whether the library chdirs itself or whether it obligates
callers to chdir before calling into the library, the bottom line is
that it requires a change to global state to function correctly, and
that change can confuse other threads.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC 4/6] git-check-attr: Normalize paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> If I understand you correctly, the use of some API routines requires a
> chdir by the caller (i.e., the surrounding application) *before* calling
> into the routine.  This is certainly a bit cleaner than the library
> chdiring itself, but it is still unusable in a multithreaded context.

Why?

Presumably you know what your threads are doing, so if you take input from
the end user after you started the environment, you will be doing the
prefix discovery and pathspec prefixing on the entry and prefix stripping
upon output but do not have to (and should not be doing) chdir at all.

Re: [RFC 4/6] git-check-attr: Normalize paths

[Michael Haggerty]

On 08/05/2011 05:02 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If I understand you correctly, the use of some API routines requires a
>> chdir by the caller (i.e., the surrounding application) *before* calling
>> into the routine.  This is certainly a bit cleaner than the library
>> chdiring itself, but it is still unusable in a multithreaded context.
> 
> Why?
> 
> Presumably you know what your threads are doing, so if you take input from
> the end user after you started the environment, you will be doing the
> prefix discovery and pathspec prefixing on the entry and prefix stripping
> upon output but do not have to (and should not be doing) chdir at all.

I must have misunderstood your earlier message.  Indeed, if none of the
git functions that one would want to libify require that CWD==project
root, then all is OK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

This patch and the following one are purely to allow testing of some
path manipulation commands.  (Perhaps they will inspire somebody to
write unit tests for absolute_path() and or prefix_path(), or document
prefix_path().)  If it is not thought that they will be useful in the
future, they can be discarded.

 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

[RFC 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..471406c 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")) {
+		int nongit_ok;
+		setup_git_directory_gently(&nongit_ok);
+		char *prefix = argv[2];
+		int prefix_len = strlen(prefix);
+		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

Re: [RFC 0/6] git-check-attr should work for relative paths

[Junio C Hamano]

A minor compilation fix-up on top, to be squashed when the series is
re-rolled.

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 23a6e07..8155b3d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -44,7 +44,7 @@ static void output_attr(int cnt, struct git_attr_value *check,
 static void check_attr(const char *prefix, int cnt,
 	struct git_attr_value *check, const char *file)
 {
-	const char *full_path =
+	char *full_path =
 		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
 	if (check != NULL) {
 		if (git_checkattr(full_path, cnt, check))
diff --git a/test-path-utils.c b/test-path-utils.c
index 471406c..3bc20e9 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -36,10 +36,10 @@ int main(int argc, char **argv)
 	}
 
 	if (argc >= 4 && !strcmp(argv[1], "prefix_path")) {
-		int nongit_ok;
-		setup_git_directory_gently(&nongit_ok);
 		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--;

Re: [RFC 0/6] git-check-attr should work for relative paths

[Michael Haggerty]

On 08/03/2011 12:02 AM, Junio C Hamano wrote:
> A minor compilation fix-up on top, to be squashed when the series is
> re-rolled. [...]

Thanks for catching this.  I will include it in the re-roll.  (The patch
series needs to be re-rolled anyway, since it depends on
mh/check-attr-listing, which is being improved based on your feedback.)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/