{"thread":{"id":"27943","subject":"[RFC 5/6] test-path-utils: Add subcommand \"absolute_path\"","startedAt":"2011-07-28T10:36:59Z","lastAt":"2011-08-07T04:32:08Z","messageCount":15,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"172254","messageId":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":null,"subject":"[RFC 0/6] git-check-attr should work for relative paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:36:59Z","receivedAt":"2011-07-28T10:36:59Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, git-check-attr gets confused if it is passed certain types\nof unnormalized paths or is invoked from a non-top-level directory.\nFor example, using the test repo created by t0003-attributes.sh,\ninquiring about a path that starts with \"./\" causes git-check-attr to\nemit a spurious warning:\n\n    $ git check-attr test ./f\n    [attr]notest !test\n     not allowed: ./.gitattributes:1\n    ./f: test: f\n    $ git check-attr test ./a/i\n    [attr]notest !test\n     not allowed: ./.gitattributes:1\n    ./a/i: test: a/i\n\nIn these cases it seems to find the right values, even including the\napplication of macros from the top-level .gitattributes file.  (My\nguess is that it is loading the top-level .gitattributes file\ntwice--once as a top-level file, and once believing that it is in a\nsubdirectory.)\n\nInvoking git-check-attr from a subdirectory using a relative path\ngives incorrect answers; for example:\n\n    $ (cd a; git check-attr test i )\n    i: test: unspecified\n\n(On the other hand, if invoked using the full path of a file, it\n\"works\":\n\n    $ (cd a; git check-attr test a/i )\n    a/i: test: a/i\n\n.)  I think that this behavior is confusing and inconsistent with\nother git commands.  It is also inconsistent with the behavior of\n.gitignore, which works from subdirectories.\n\nThe patches in this series add tests demonstrating the problem,\npropose a possible solution, and add a couple of subcommands to\ntest-path-utils.\n\nI am far from confident that the solution proposed in patch 4 is\ncorrect; please see the notes in that email for discussion.\n\nThe changes to test-path-utils helped me figure out how the\ncorresponding functions work (comments? we don't need no stinkin'\ncomments) but they are not used anywhere.  Take 'em or leave 'em.\n\nMichael Haggerty (6):\n  git-check-attr: test that no output is written to stderr\n  git-check-attr: Demonstrate problems with unnormalized paths\n  git-check-attr: Demonstrate problems with relative paths\n  git-check-attr: Normalize paths\n  test-path-utils: Add subcommand \"absolute_path\"\n  test-path-utils: Add subcommand \"prefix_path\"\n\n builtin/check-attr.c  |   20 ++++++++++++--------\n t/t0003-attributes.sh |   29 ++++++++++++++++++++++++++---\n test-path-utils.c     |   22 ++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 11 deletions(-)\n\n-- \n1.7.6.8.gd2879\n"},{"id":"172249","messageId":"1311849425-9057-2-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 1/6] git-check-attr: test that no output is written to stderr","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:00Z","receivedAt":"2011-07-28T10:37:00Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0003-attributes.sh |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 5acb2d5..4f2fbc0 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -9,9 +9,10 @@ attr_check () {\n \tpath=\"$1\"\n \texpect=\"$2\"\n \n-\tgit check-attr test -- \"$path\" >actual &&\n+\tgit check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $2\" >expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_line_count = 0 err\n \n }\n \n-- \n1.7.6.8.gd2879\n"},{"id":"172251","messageId":"1311849425-9057-3-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 2/6] git-check-attr: Demonstrate problems with unnormalized paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:01Z","receivedAt":"2011-07-28T10:37:01Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0003-attributes.sh |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 4f2fbc0..43957ea 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -93,6 +93,15 @@ test_expect_success 'attribute test' '\n \n '\n \n+test_expect_failure 'unnormalized paths' '\n+\n+\tattr_check ./f f &&\n+\tattr_check ./a/g a/g &&\n+\tattr_check a/./g a/g &&\n+\tattr_check a/c/../b/g a/b/g\n+\n+'\n+\n test_expect_success 'core.attributesfile' '\n \tattr_check global unspecified &&\n \tgit config core.attributesfile \"$HOME/global-gitattributes\" &&\n-- \n1.7.6.8.gd2879\n"},{"id":"172252","messageId":"1311849425-9057-4-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 3/6] git-check-attr: Demonstrate problems with relative paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:02Z","receivedAt":"2011-07-28T10:37:02Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0003-attributes.sh |   15 ++++++++++++++-\n 1 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 43957ea..f6cf77d 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -19,7 +19,7 @@ attr_check () {\n \n test_expect_success 'setup' '\n \n-\tmkdir -p a/b/d a/c &&\n+\tmkdir -p a/b/d a/c b &&\n \t(\n \t\techo \"[attr]notest !test\"\n \t\techo \"f\ttest=f\"\n@@ -102,6 +102,19 @@ test_expect_failure 'unnormalized paths' '\n \n '\n \n+test_expect_failure 'relative paths' '\n+\n+\t(cd a && attr_check ../f f) &&\n+\t(cd a && attr_check f f) &&\n+\t(cd a && attr_check i a/i) &&\n+\t(cd a && attr_check g a/g) &&\n+\t(cd a && attr_check b/g a/b/g) &&\n+\t(cd b && attr_check ../a/f f) &&\n+\t(cd b && attr_check ../a/g a/g) &&\n+\t(cd b && attr_check ../a/b/g a/b/g)\n+\n+'\n+\n test_expect_success 'core.attributesfile' '\n \tattr_check global unspecified &&\n \tgit config core.attributesfile \"$HOME/global-gitattributes\" &&\n-- \n1.7.6.8.gd2879\n"},{"id":"172253","messageId":"1311849425-9057-5-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 4/6] git-check-attr: Normalize paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:03Z","receivedAt":"2011-07-28T10:37:03Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Normalize the path arguments (relative to the working tree root, if\napplicable) before looking up their attributes.  This requires passing\nthe prefix down the call chain.\n\nThis fixes two test cases for different reasons:\n\n* \"unnormalized paths\" is fixed because the .gitattribute-file-seeking\n  code is not confused into reading the top-level file twice.\n\n* \"relative paths\" is fixed because the canonical pathnames are passed\n  to get_checkattr() or get_allattrs(), allowing them to match the\n  pathname patterns as expected.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nI had a very hard time figuring out the correct approach to fixing\nthis problem.  There is so little documentation in the C code!  The\nsuggested patch fixes the tests, but it might be wrong for other\nreasons:\n\n1. I'm not sure whether it is correct to fix this problem at the level\n   of git-check-attr, or whether the fix belongs in the API layer.\n   What is the convention for API functions?  Do they typically take\n   path names relative to the CWD or relative to the working tree\n   root, or ...?  I also found examples of commands that change the\n   current directory during their operation, making the CWD and the\n   working tree root identical.  What is preferred?  (And where is it\n   documented?)\n\n2. It could be that I'm abusing the prefix_path() function, or that\n   there is a better way to achieve the normalization.\n\n builtin/check-attr.c  |   20 ++++++++++++--------\n t/t0003-attributes.sh |    4 ++--\n 2 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 781b7df..23a6e07 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -41,22 +41,26 @@ static void output_attr(int cnt, struct git_attr_value *check,\n \t}\n }\n \n-static void check_attr(int cnt, struct git_attr_value *check,\n-\tconst char *file)\n+static void check_attr(const char *prefix, int cnt,\n+\tstruct git_attr_value *check, const char *file)\n {\n+\tconst char *full_path =\n+\t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \tif (check != NULL) {\n-\t\tif (git_checkattr(file, cnt, check))\n+\t\tif (git_checkattr(full_path, cnt, check))\n \t\t\tdie(\"git_checkattr died\");\n \t\toutput_attr(cnt, check, file);\n \t} else {\n-\t\tif (git_allattrs(file, &cnt, &check))\n+\t\tif (git_allattrs(full_path, &cnt, &check))\n \t\t\tdie(\"git_allattrs died\");\n \t\toutput_attr(cnt, check, file);\n \t\tfree(check);\n \t}\n+\tfree(full_path);\n }\n \n-static void check_attr_stdin_paths(int cnt, struct git_attr_value *check)\n+static void check_attr_stdin_paths(const char *prefix, int cnt,\n+\tstruct git_attr_value *check)\n {\n \tstruct strbuf buf, nbuf;\n \tint line_termination = null_term_line ? 0 : '\\n';\n@@ -70,7 +74,7 @@ static void check_attr_stdin_paths(int cnt, struct git_attr_value *check)\n \t\t\t\tdie(\"line is badly quoted\");\n \t\t\tstrbuf_swap(&buf, &nbuf);\n \t\t}\n-\t\tcheck_attr(cnt, check, buf.buf);\n+\t\tcheck_attr(prefix, cnt, check, buf.buf);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \tstrbuf_release(&buf);\n@@ -154,10 +158,10 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (stdin_paths)\n-\t\tcheck_attr_stdin_paths(cnt, check);\n+\t\tcheck_attr_stdin_paths(prefix, cnt, check);\n \telse {\n \t\tfor (i = filei; i < argc; i++)\n-\t\t\tcheck_attr(cnt, check, argv[i]);\n+\t\t\tcheck_attr(prefix, cnt, check, argv[i]);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \treturn 0;\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex f6cf77d..ae2f1da 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -93,7 +93,7 @@ test_expect_success 'attribute test' '\n \n '\n \n-test_expect_failure 'unnormalized paths' '\n+test_expect_success 'unnormalized paths' '\n \n \tattr_check ./f f &&\n \tattr_check ./a/g a/g &&\n@@ -102,7 +102,7 @@ test_expect_failure 'unnormalized paths' '\n \n '\n \n-test_expect_failure 'relative paths' '\n+test_expect_success 'relative paths' '\n \n \t(cd a && attr_check ../f f) &&\n \t(cd a && attr_check f f) &&\n-- \n1.7.6.8.gd2879\n"},{"id":"172248","messageId":"1311849425-9057-6-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 5/6] test-path-utils: Add subcommand \"absolute_path\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:04Z","receivedAt":"2011-07-28T10:37:04Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nThis patch and the following one are purely to allow testing of some\npath manipulation commands.  (Perhaps they will inspire somebody to\nwrite unit tests for absolute_path() and or prefix_path(), or document\nprefix_path().)  If it is not thought that they will be useful in the\nfuture, they can be discarded.\n\n test-path-utils.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex e767159..a410e30 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -20,6 +20,15 @@ int main(int argc, char **argv)\n \t\treturn 0;\n \t}\n \n+\tif (argc >= 2 && !strcmp(argv[1], \"absolute_path\")) {\n+\t\twhile (argc > 2) {\n+\t\t\tputs(absolute_path(argv[2]));\n+\t\t\targc--;\n+\t\t\targv++;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \tif (argc == 4 && !strcmp(argv[1], \"longest_ancestor_length\")) {\n \t\tint len = longest_ancestor_length(argv[2], argv[3]);\n \t\tprintf(\"%d\\n\", len);\n-- \n1.7.6.8.gd2879\n"},{"id":"172250","messageId":"1311849425-9057-7-git-send-email-mhagger@alum.mit.edu","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 6/6] test-path-utils: Add subcommand \"prefix_path\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-07-28T10:37:05Z","receivedAt":"2011-07-28T10:37:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n test-path-utils.c |   13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex a410e30..471406c 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -35,6 +35,19 @@ int main(int argc, char **argv)\n \t\treturn 0;\n \t}\n \n+\tif (argc >= 4 && !strcmp(argv[1], \"prefix_path\")) {\n+\t\tint nongit_ok;\n+\t\tsetup_git_directory_gently(&nongit_ok);\n+\t\tchar *prefix = argv[2];\n+\t\tint prefix_len = strlen(prefix);\n+\t\twhile (argc > 3) {\n+\t\t\tputs(prefix_path(prefix, prefix_len, argv[3]));\n+\t\t\targc--;\n+\t\t\targv++;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \tif (argc == 4 && !strcmp(argv[1], \"strip_path_suffix\")) {\n \t\tchar *prefix = strip_path_suffix(argv[2], argv[3]);\n \t\tprintf(\"%s\\n\", prefix ? prefix : \"(null)\");\n-- \n1.7.6.8.gd2879\n"},{"id":"172676","messageId":"7v4o1zg20r.fsf@alter.siamese.dyndns.org","threadId":"27943","inReplyTo":"1311849425-9057-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T17:24:20Z","receivedAt":"2011-08-02T17:24:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> 1. I'm not sure whether it is correct to fix this problem at the level\n>    of git-check-attr, or whether the fix belongs in the API layer.\n>    What is the convention for API functions?  Do they typically take\n>    path names relative to the CWD or relative to the working tree\n>    root, or ...?\n\nI think passing down \"prefix\" (i.e. where your $(cwd) was relative to the\nroot level of the working tree) and the user-supplied \"pathspec\" (which\ntypically is relative to that original $(cwd)) is the canonical approach.\nThe very original git worked only at the root level of the working tree,\nwith paths specified relative to the root level of the tree, so many code\ndo:\n\n\t- find out the root of the working tree;\n        - note where the $(cwd) was in \"prefix\";\n        - chdir to the root of the working tree;\n\t- prepend the \"prefix\" to user supplied pathspec;\n        - forget all the complexity and work on the whole tree.\n\nThen the \"prefix\" gets stripped away from the beginning of the paths when\nreporting.\n\nYour patch from a cursory look seems to follow that pattern, which is\ngood.\n"},{"id":"172704","messageId":"7vwrevcw12.fsf@alter.siamese.dyndns.org","threadId":"27943","inReplyTo":"1311849425-9057-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [RFC 0/6] git-check-attr should work for relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T22:02:01Z","receivedAt":"2011-08-02T22:02:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A minor compilation fix-up on top, to be squashed when the series is\nre-rolled.\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 23a6e07..8155b3d 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -44,7 +44,7 @@ static void output_attr(int cnt, struct git_attr_value *check,\n static void check_attr(const char *prefix, int cnt,\n \tstruct git_attr_value *check, const char *file)\n {\n-\tconst char *full_path =\n+\tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \tif (check != NULL) {\n \t\tif (git_checkattr(full_path, cnt, check))\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex 471406c..3bc20e9 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -36,10 +36,10 @@ int main(int argc, char **argv)\n \t}\n \n \tif (argc >= 4 && !strcmp(argv[1], \"prefix_path\")) {\n-\t\tint nongit_ok;\n-\t\tsetup_git_directory_gently(&nongit_ok);\n \t\tchar *prefix = argv[2];\n \t\tint prefix_len = strlen(prefix);\n+\t\tint nongit_ok;\n+\t\tsetup_git_directory_gently(&nongit_ok);\n \t\twhile (argc > 3) {\n \t\t\tputs(prefix_path(prefix, prefix_len, argv[3]));\n \t\t\targc--;\n"},{"id":"172847","messageId":"4E3A12B9.1010800@alum.mit.edu","threadId":"27943","inReplyTo":"7v4o1zg20r.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-04T03:32:09Z","receivedAt":"2011-08-04T03:32:09Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/02/2011 07:24 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> 1. I'm not sure whether it is correct to fix this problem at the level\n>>    of git-check-attr, or whether the fix belongs in the API layer.\n>>    What is the convention for API functions?  Do they typically take\n>>    path names relative to the CWD or relative to the working tree\n>>    root, or ...?\n> \n> I think passing down \"prefix\" (i.e. where your $(cwd) was relative to the\n> root level of the working tree) and the user-supplied \"pathspec\" (which\n> typically is relative to that original $(cwd)) is the canonical approach.\n> The very original git worked only at the root level of the working tree,\n> with paths specified relative to the root level of the tree, so many code\n> do:\n> \n> \t- find out the root of the working tree;\n>         - note where the $(cwd) was in \"prefix\";\n>         - chdir to the root of the working tree;\n> \t- prepend the \"prefix\" to user supplied pathspec;\n>         - forget all the complexity and work on the whole tree.\n> \n> Then the \"prefix\" gets stripped away from the beginning of the paths when\n> reporting.\n> \n> Your patch from a cursory look seems to follow that pattern, which is\n> good.\n\nThanks for the explanation.\n\nYes, my code follows the pattern, except that in this case it is\nunnecessary to chdir to the root of the working tree.\n\nAll this chdiring is going to be a nightmare for the libification of\ngit, since the cwd is a program-wide global variable with implicit\nside-effects on almost any code that deals with the filesystem.  It is\nobviously not permissible for a library to change directories (not even\ntemporarily, if the library is intended to be used in a multithreaded\napplication).  But that is a topic for another day.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"172848","messageId":"4E3A1374.2050000@alum.mit.edu","threadId":"27943","inReplyTo":"7vwrevcw12.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 0/6] git-check-attr should work for relative paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-04T03:35:16Z","receivedAt":"2011-08-04T03:35:16Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/03/2011 12:02 AM, Junio C Hamano wrote:\n> A minor compilation fix-up on top, to be squashed when the series is\n> re-rolled. [...]\n\nThanks for catching this.  I will include it in the re-roll.  (The patch\nseries needs to be re-rolled anyway, since it depends on\nmh/check-attr-listing, which is being improved based on your feedback.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"172933","messageId":"7vliv95cpb.fsf@alter.siamese.dyndns.org","threadId":"27943","inReplyTo":"4E3A12B9.1010800@alum.mit.edu","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-04T17:05:52Z","receivedAt":"2011-08-04T17:05:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> I think passing down \"prefix\" (i.e. where your $(cwd) was relative to the\n>> root level of the working tree) and the user-supplied \"pathspec\" (which\n>> typically is relative to that original $(cwd)) is the canonical approach.\n>> The very original git worked only at the root level of the working tree,\n>> with paths specified relative to the root level of the tree, so many code\n>> do:\n>> \n>> \t- find out the root of the working tree;\n>>         - note where the $(cwd) was in \"prefix\";\n>>         - chdir to the root of the working tree;\n>> \t- prepend the \"prefix\" to user supplied pathspec;\n>>         - forget all the complexity and work on the whole tree.\n>> \n>> Then the \"prefix\" gets stripped away from the beginning of the paths when\n>> reporting.\n>> \n>> Your patch from a cursory look seems to follow that pattern, which is\n>> good.\n>\n> Thanks for the explanation.\n>\n> Yes, my code follows the pattern, except that in this case it is\n> unnecessary to chdir to the root of the working tree.\n\nJust to make sure there is no misunderstanding. The chdir() should not be\nin the core part of the system that you may want to libify.\n\nThe above pattern was developed primarily so that older utility functions\nin the system that were written back when nobody ran git from anywhere\nother than the top level of the working tree can be easily adapted to main\nprograms that can be launched from a subdirectory. The initial set-up part\nof the program is responsible for figuring out \"prefix\", turning relative\npaths given by the user into paths relative to the top of the working\ntree, and then chdir'ing to the top.\n\nAfter all that happens, the library-ish parts of the system only have to\ndeal with full paths relative to the root of the working tree. \"prefix\"\ncomes into play when reporting the results (i.e. showing paths relative to\nuser's $(cwd) in the output or in the error messages).\n"},{"id":"172962","messageId":"4E3B8CAA.4030002@alum.mit.edu","threadId":"27943","inReplyTo":"7vliv95cpb.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-05T06:24:42Z","receivedAt":"2011-08-05T06:24:42Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/04/2011 07:05 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>>> I think passing down \"prefix\" (i.e. where your $(cwd) was relative to the\n>>> root level of the working tree) and the user-supplied \"pathspec\" (which\n>>> typically is relative to that original $(cwd)) is the canonical approach.\n>>> The very original git worked only at the root level of the working tree,\n>>> with paths specified relative to the root level of the tree, so many code\n>>> do:\n>>>\n>>> \t- find out the root of the working tree;\n>>>         - note where the $(cwd) was in \"prefix\";\n>>>         - chdir to the root of the working tree;\n>>> \t- prepend the \"prefix\" to user supplied pathspec;\n>>>         - forget all the complexity and work on the whole tree.\n>>>\n>>> Then the \"prefix\" gets stripped away from the beginning of the paths when\n>>> reporting.\n>>>\n>>> Your patch from a cursory look seems to follow that pattern, which is\n>>> good.\n>>\n>> Thanks for the explanation.\n>>\n>> Yes, my code follows the pattern, except that in this case it is\n>> unnecessary to chdir to the root of the working tree.\n> \n> Just to make sure there is no misunderstanding. The chdir() should not be\n> in the core part of the system that you may want to libify.\n> \n> The above pattern was developed primarily so that older utility functions\n> in the system that were written back when nobody ran git from anywhere\n> other than the top level of the working tree can be easily adapted to main\n> programs that can be launched from a subdirectory. The initial set-up part\n> of the program is responsible for figuring out \"prefix\", turning relative\n> paths given by the user into paths relative to the top of the working\n> tree, and then chdir'ing to the top.\n> \n> After all that happens, the library-ish parts of the system only have to\n> deal with full paths relative to the root of the working tree. \"prefix\"\n> comes into play when reporting the results (i.e. showing paths relative to\n> user's $(cwd) in the output or in the error messages).\n\nIf I understand you correctly, the use of some API routines requires a\nchdir by the caller (i.e., the surrounding application) *before* calling\ninto the routine.  This is certainly a bit cleaner than the library\nchdiring itself, but it is still unusable in a multithreaded context.\nRegardless of whether the library chdirs itself or whether it obligates\ncallers to chdir before calling into the library, the bottom line is\nthat it requires a change to global state to function correctly, and\nthat change can confuse other threads.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"172978","messageId":"7vaabn52bb.fsf@alter.siamese.dyndns.org","threadId":"27943","inReplyTo":"4E3B8CAA.4030002@alum.mit.edu","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-05T15:02:32Z","receivedAt":"2011-08-05T15:02:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> If I understand you correctly, the use of some API routines requires a\n> chdir by the caller (i.e., the surrounding application) *before* calling\n> into the routine.  This is certainly a bit cleaner than the library\n> chdiring itself, but it is still unusable in a multithreaded context.\n\nWhy?\n\nPresumably you know what your threads are doing, so if you take input from\nthe end user after you started the environment, you will be doing the\nprefix discovery and pathspec prefixing on the entry and prefix stripping\nupon output but do not have to (and should not be doing) chdir at all.\n"},{"id":"173076","messageId":"4E3E1548.3080502@alum.mit.edu","threadId":"27943","inReplyTo":"7vaabn52bb.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 4/6] git-check-attr: Normalize paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-07T04:32:08Z","receivedAt":"2011-08-07T04:32:08Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/05/2011 05:02 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> If I understand you correctly, the use of some API routines requires a\n>> chdir by the caller (i.e., the surrounding application) *before* calling\n>> into the routine.  This is certainly a bit cleaner than the library\n>> chdiring itself, but it is still unusable in a multithreaded context.\n> \n> Why?\n> \n> Presumably you know what your threads are doing, so if you take input from\n> the end user after you started the environment, you will be doing the\n> prefix discovery and pathspec prefixing on the entry and prefix stripping\n> upon output but do not have to (and should not be doing) chdir at all.\n\nI must have misunderstood your earlier message.  Indeed, if none of the\ngit functions that one would want to libify require that CWD==project\nroot, then all is OK.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}