{"thread":{"id":"40479","subject":"[PATCH 1/2] test-path-utils.c: remove incorrect assumption","startedAt":"2015-10-03T12:44:43Z","lastAt":"2015-10-09T10:12:08Z","messageCount":9,"participants":["Ray Donnelly","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"271057","messageId":"CAOYw7dubGJ=m5+EnjGy7jTQxR+b0uBmyG138KEQ5rzX2K7WcgA@mail.gmail.com","threadId":"40479","inReplyTo":null,"subject":"[PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-03T12:44:43Z","receivedAt":"2015-10-03T12:44:43Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"In normalize_ceiling_entry(), we test that normalized paths end with\nslash, *unless* the path to be normalized was already the root\ndirectory.\n\nHowever, normalize_path_copy() does not even enforce this condition.\n\nEven worse: on Windows, the root directory gets translated into a\nWindows directory by the Bash before being passed to `git.exe` (or\n`test-path-utils.exe`), which means that we cannot even know whether\nthe path that was passed to us was the root directory to begin with.\n\nThis issue has already caused endless hours of trying to \"fix\" the\nMSYS2 runtime, only to break other things due to MSYS2 ensuring that\nthe converted path maintains the same state as the input path with\nrespect to any final '/'.\n\nSo let's just forget about this test. It is non-essential to Git's\noperation, anyway.\n\nAck-by: Johannes Schindelin \nSigned-off-by: Ray Donnelly \n---\n test-path-utils.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex 3dd3744..c67bf65 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct\nstring_list_item *item, void *unused)\n if (normalize_path_copy(buf, ceil) < 0)\n die(\"Path \\\"%s\\\" could not be normalized\", ceil);\n len = strlen(buf);\n- if (len > 1 && buf[len-1] == '/')\n- die(\"Normalized path \\\"%s\\\" ended with slash\", buf);\n free(item->string);\n item->string = xstrdup(buf);\n return 1;\n-- \n2.5.2\n"},{"id":"271058","messageId":"CAOYw7dvYvgXWNi=kFdB0kXP0BjGTmcY-dG6mkaKU93LdV4i5HQ@mail.gmail.com","threadId":"40479","inReplyTo":"CAOYw7dubGJ=m5+EnjGy7jTQxR+b0uBmyG138KEQ5rzX2K7WcgA@mail.gmail.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-03T15:38:47Z","receivedAt":"2015-10-03T15:38:47Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"I'm going to have to attach this as a file, git-send-email isn't\nworking for me; apologies.\n\nOn Sat, Oct 3, 2015 at 1:44 PM, Ray Donnelly wrote:\n> In normalize_ceiling_entry(), we test that normalized paths end with\n> slash, *unless* the path to be normalized was already the root\n> directory.\n>\n> However, normalize_path_copy() does not even enforce this condition.\n>\n> Even worse: on Windows, the root directory gets translated into a\n> Windows directory by the Bash before being passed to `git.exe` (or\n> `test-path-utils.exe`), which means that we cannot even know whether\n> the path that was passed to us was the root directory to begin with.\n>\n> This issue has already caused endless hours of trying to \"fix\" the\n> MSYS2 runtime, only to break other things due to MSYS2 ensuring that\n> the converted path maintains the same state as the input path with\n> respect to any final '/'.\n>\n> So let's just forget about this test. It is non-essential to Git's\n> operation, anyway.\n>\n> Ack-by: Johannes Schindelin \n> Signed-off-by: Ray Donnelly \n> ---\n> test-path-utils.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/test-path-utils.c b/test-path-utils.c\n> index 3dd3744..c67bf65 100644\n> --- a/test-path-utils.c\n> +++ b/test-path-utils.c\n> @@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct\n> string_list_item *item, void *unused)\n> if (normalize_path_copy(buf, ceil) < 0)\n> die(\"Path \\\"%s\\\" could not be normalized\", ceil);\n> len = strlen(buf);\n> - if (len > 1 && buf[len-1] == '/')\n> - die(\"Normalized path \\\"%s\\\" ended with slash\", buf);\n> free(item->string);\n> item->string = xstrdup(buf);\n> return 1;\n> --\n> 2.5.2\n\n\nFrom 3deea12dd8506f88fdaeabcc33683f81b75a13fa Mon Sep 17 00:00:00 2001\nFrom: Ray Donnelly \nDate: Thu, 1 Oct 2015 20:04:17 +0100\nSubject: [PATCH 1/2] test-path-utils.c: remove incorrect assumption\n\nIn normalize_ceiling_entry(), we test that normalized paths end with\nslash, *unless* the path to be normalized was already the root\ndirectory.\n\nHowever, normalize_path_copy() does not even enforce this condition.\n\nEven worse: on Windows, the root directory gets translated into a\nWindows directory by the Bash before being passed to `git.exe` (or\n`test-path-utils.exe`), which means that we cannot even know whether\nthe path that was passed to us was the root directory to begin with.\n\nThis issue has already caused endless hours of trying to \"fix\" the\nMSYS2 runtime, only to break other things due to MSYS2 ensuring that\nthe converted path maintains the same state as the input path with\nrespect to any final '/'.\n\nSo let's just forget about this test. It is non-essential to Git's\noperation, anyway.\n\nAck-by: Johannes Schindelin \nSigned-off-by: Ray Donnelly \n---\n test-path-utils.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex 3dd3744..c67bf65 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct string_list_item *item, void *unused)\n \tif (normalize_path_copy(buf, ceil) < 0)\n \t\tdie(\"Path \\\"%s\\\" could not be normalized\", ceil);\n \tlen = strlen(buf);\n-\tif (len > 1 && buf[len-1] == '/')\n-\t\tdie(\"Normalized path \\\"%s\\\" ended with slash\", buf);\n \tfree(item->string);\n \titem->string = xstrdup(buf);\n \treturn 1;\n-- \n2.5.2\n\n"},{"id":"271063","messageId":"xmqqlhbj3mfo.fsf@gitster.mtv.corp.google.com","threadId":"40479","inReplyTo":"CAOYw7dubGJ=m5+EnjGy7jTQxR+b0uBmyG138KEQ5rzX2K7WcgA@mail.gmail.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-03T17:13:15Z","receivedAt":"2015-10-03T17:13:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ray Donnelly writes:\n\n> In normalize_ceiling_entry(), we test that normalized paths end with\n> slash, *unless* the path to be normalized was already the root\n> directory.\n>\n> However, normalize_path_copy() does not even enforce this condition.\n\nPerhaps the real issue to be addressed is the above, and your patch\nis killing a coalmine canary?\n\nSome callers of this function in real code (i.e. not the one you are\nremoving the check) do seem to depend on that condition, e.g. the\ncodepath in clone that leads to add_to_alternates_file() wants to\nmake sure it does not add an duplicate, so it may end up not noticing\n/foo/bar and /foo/bar/ are the same thing, no? There may be others.\n"},{"id":"271086","messageId":"CAOYw7dv4iPQ4cq4Ab1ZeThrp=u51T5v387a1Y8QPO-yj=fyMcg@mail.gmail.com","threadId":"40479","inReplyTo":"xmqqlhbj3mfo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-04T14:51:22Z","receivedAt":"2015-10-04T14:51:22Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"On Sat, Oct 3, 2015 at 6:13 PM, Junio C Hamano wrote:\n> Ray Donnelly writes:\n>\n>> In normalize_ceiling_entry(), we test that normalized paths end with\n>> slash, *unless* the path to be normalized was already the root\n>> directory.\n>>\n>> However, normalize_path_copy() does not even enforce this condition.\n>\n> Perhaps the real issue to be addressed is the above, and your patch\n> is killing a coalmine canary?\n>\n> Some callers of this function in real code (i.e. not the one you are\n> removing the check) do seem to depend on that condition, e.g. the\n> codepath in clone that leads to add_to_alternates_file() wants to\n> make sure it does not add an duplicate, so it may end up not noticing\n> /foo/bar and /foo/bar/ are the same thing, no? There may be others.\n>\n>\n\nEnforcing that normalize_path_copy() removes any trailing '/' (apart\nfrom the root directory) breaks other things that assume it doesn't\nmess with trailing '/'s, for example filtering in ls-tree. Any\nsuggestions for what to do about this? Would a flag be appropriate as\nto whether to do this part or not? Though I'll admit I don't like the\nidea of adding flags to modify the behavior of something that's meant\nto \"normalize\" something. Alternatively, I could go through all the\nbreakages and try to fix them up?\n"},{"id":"271089","messageId":"xmqqwpv21rej.fsf@gitster.mtv.corp.google.com","threadId":"40479","inReplyTo":"CAOYw7dv4iPQ4cq4Ab1ZeThrp=u51T5v387a1Y8QPO-yj=fyMcg@mail.gmail.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-04T17:21:08Z","receivedAt":"2015-10-04T17:21:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ray Donnelly writes:\n\n>> Some callers of this function in real code (i.e. not the one you are\n>> removing the check) do seem to depend on that condition, e.g. the\n>> codepath in clone that leads to add_to_alternates_file() wants to\n>> make sure it does not add an duplicate, so it may end up not noticing\n>> /foo/bar and /foo/bar/ are the same thing, no? There may be others.\n>\n> Enforcing that normalize_path_copy() removes any trailing '/' (apart\n> from the root directory) breaks other things that assume it doesn't\n> mess with trailing '/'s, for example filtering in ls-tree. Any\n> suggestions for what to do about this? Would a flag be appropriate as\n> to whether to do this part or not? Though I'll admit I don't like the\n> idea of adding flags to modify the behavior of something that's meant\n> to \"normalize\" something. Alternatively, I could go through all the\n> breakages and try to fix them up?\n\nI agree with you that \"normalize\" should \"normalize\". Making sure\nthat all the callers expect the same kind of normalization would be\na lot of work but I do think that is the best approach in the long\nrun. Thanks for the ls-tree example, by the way, did you find it by\ncode inspection? I do not think it is reasonable to expect the test\ncoverage for this to be 100%, so the \"try to fix them up\" would have\nto involve a lot of manual work both in fixing and reviewing,\nunfortunately.\n\nThe first step of the \"best approach\" would be to make a note on\nnormalize_path_copy() by adding a NEEDSWORK: comment to describe the\nsituation.\n\nThanks.\n"},{"id":"271111","messageId":"CAOYw7duDLWYpu+NK2t2+hV3rtU=dK3eQ6R11mfwLKbQQowbWuQ@mail.gmail.com","threadId":"40479","inReplyTo":"xmqqwpv21rej.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-04T23:36:16Z","receivedAt":"2015-10-04T23:36:16Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"On Sun, Oct 4, 2015 at 6:21 PM, Junio C Hamano wrote:\n> Ray Donnelly writes:\n>\n>>> Some callers of this function in real code (i.e. not the one you are\n>>> removing the check) do seem to depend on that condition, e.g. the\n>>> codepath in clone that leads to add_to_alternates_file() wants to\n>>> make sure it does not add an duplicate, so it may end up not noticing\n>>> /foo/bar and /foo/bar/ are the same thing, no? There may be others.\n>>\n>> Enforcing that normalize_path_copy() removes any trailing '/' (apart\n>> from the root directory) breaks other things that assume it doesn't\n>> mess with trailing '/'s, for example filtering in ls-tree. Any\n>> suggestions for what to do about this? Would a flag be appropriate as\n>> to whether to do this part or not? Though I'll admit I don't like the\n>> idea of adding flags to modify the behavior of something that's meant\n>> to \"normalize\" something. Alternatively, I could go through all the\n>> breakages and try to fix them up?\n>\n> I agree with you that \"normalize\" should \"normalize\". Making sure\n> that all the callers expect the same kind of normalization would be\n> a lot of work but I do think that is the best approach in the long\n> run. Thanks for the ls-tree example, by the way, did you find it by\n> code inspection? I do not think it is reasonable to expect the test\n> coverage for this to be 100%, so the \"try to fix them up\" would have\n> to involve a lot of manual work both in fixing and reviewing,\n> unfortunately.\n\nFor the ls-tree failure, I ran \"make test\" to see how much fell out.\nI'm not familiar with the code-base yet, so I figured that at least\ninvestigating the changes needed to make the test-suite pass would be\na good entry point to reading the code; I will study it at the same\ntime to try and get my bearings.\n\n>\n> The first step of the \"best approach\" would be to make a note on\n> normalize_path_copy() by adding a NEEDSWORK: comment to describe the\n> situation.\n>\n> Thanks.\n"},{"id":"271355","messageId":"CAOYw7dsfKpQT4NXjKrNRVsoPCrAFDjp7Hnms_5SF7JLw6s9g-Q@mail.gmail.com","threadId":"40479","inReplyTo":"CAOYw7duDLWYpu+NK2t2+hV3rtU=dK3eQ6R11mfwLKbQQowbWuQ@mail.gmail.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-08T20:42:45Z","receivedAt":"2015-10-08T20:42:45Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"On Mon, Oct 5, 2015 at 12:36 AM, Ray Donnelly wrote:\n> On Sun, Oct 4, 2015 at 6:21 PM, Junio C Hamano wrote:\n>> Ray Donnelly writes:\n>>\n>>>> Some callers of this function in real code (i.e. not the one you are\n>>>> removing the check) do seem to depend on that condition, e.g. the\n>>>> codepath in clone that leads to add_to_alternates_file() wants to\n>>>> make sure it does not add an duplicate, so it may end up not noticing\n>>>> /foo/bar and /foo/bar/ are the same thing, no? There may be others.\n>>>\n>>> Enforcing that normalize_path_copy() removes any trailing '/' (apart\n>>> from the root directory) breaks other things that assume it doesn't\n>>> mess with trailing '/'s, for example filtering in ls-tree. Any\n>>> suggestions for what to do about this? Would a flag be appropriate as\n>>> to whether to do this part or not? Though I'll admit I don't like the\n>>> idea of adding flags to modify the behavior of something that's meant\n>>> to \"normalize\" something. Alternatively, I could go through all the\n>>> breakages and try to fix them up?\n>>\n>> I agree with you that \"normalize\" should \"normalize\". Making sure\n>> that all the callers expect the same kind of normalization would be\n>> a lot of work but I do think that is the best approach in the long\n>> run. Thanks for the ls-tree example, by the way, did you find it by\n>> code inspection? I do not think it is reasonable to expect the test\n>> coverage for this to be 100%, so the \"try to fix them up\" would have\n>> to involve a lot of manual work both in fixing and reviewing,\n>> unfortunately.\n>\n> For the ls-tree failure, I ran \"make test\" to see how much fell out.\n> I'm not familiar with the code-base yet, so I figured that at least\n> investigating the changes needed to make the test-suite pass would be\n> a good entry point to reading the code; I will study it at the same\n> time to try and get my bearings.\n>\n>>\n>> The first step of the \"best approach\" would be to make a note on\n>> normalize_path_copy() by adding a NEEDSWORK: comment to describe the\n>> situation.\n\nI hope this is acceptable for the first part of this task.\n\n>>\n>> Thanks.\n\n\nFrom e3f073b1e154f44c1e1c7716a2ed6977dd144224 Mon Sep 17 00:00:00 2001\nFrom: Ray Donnelly \nDate: Thu, 8 Oct 2015 17:10:28 +0100\nSubject: [PATCH 1/2] normalize_path_copy: NEEDSWORK for trailing '/'\n\nCurrently, normalize_path_copy () does nothing regarding trailing '/'s.\nInstead it should remove them except in the case of the root folder.\n---\n path.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/path.c b/path.c\nindex 65beb2d..4ae6db6 100644\n--- a/path.c\n+++ b/path.c\n@@ -893,6 +893,11 @@ const char *remove_leading_path(const char *in, const char *prefix)\n * normalized, any time \"../\" eats up to the prefix_len part,\n * prefix_len is reduced. In the end prefix_len is the remaining\n * prefix that has not been overridden by user pathspec.\n+ *\n+ * NEEDSWORK: This function doesn't perform normalization w.r.t. trailing '/'.\n+ * For everything but the root folder itself, the normalized path should not\n+ * end with a '/', then the callers need to be fixed up accordingly.\n+ *\n */\n int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)\n {\n-- \n2.6.1\n\n"},{"id":"271359","messageId":"xmqq1td4rgvv.fsf@gitster.mtv.corp.google.com","threadId":"40479","inReplyTo":"CAOYw7dsfKpQT4NXjKrNRVsoPCrAFDjp7Hnms_5SF7JLw6s9g-Q@mail.gmail.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T01:05:08Z","receivedAt":"2015-10-09T01:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll squash this in as part of your first patch that removes the\ntest from test-path-utils.c. That makes it clearer why it is the\nright thing to remove the test, I'd think.\n\nThanks.\n"},{"id":"271382","messageId":"CAOYw7dtC=7x8c2rUPxBbxq1OZgZB_-L3AD90riH9esYV4E85Ww@mail.gmail.com","threadId":"40479","inReplyTo":"xmqq1td4rgvv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption","fromName":"Ray Donnelly","fromEmail":"mingw.android@gmail.com","sentAt":"2015-10-09T10:12:08Z","receivedAt":"2015-10-09T10:12:08Z","isPatch":true,"sender":{"key":"mingw.android@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1042804?v=4"},"body":"On Fri, Oct 9, 2015 at 2:05 AM, Junio C Hamano wrote:\n> I'll squash this in as part of your first patch that removes the\n> test from test-path-utils.c. That makes it clearer why it is the\n> right thing to remove the test, I'd think.\n>\n\nGreat, many thanks!\n\n> Thanks.\n>\n"}]}