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

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

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

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

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

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

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.

On 08/02/2011 07:24 PM, Junio C Hamano wrote:

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> writes:

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

On 08/04/2011 07:05 PM, Junio C Hamano wrote:

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> writes:

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.

On 08/05/2011 05:02 PM, Junio C Hamano wrote:

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

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

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

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

On 08/03/2011 12:02 AM, Junio C Hamano wrote:

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