* [PATCH 1/2] test-path-utils.c: remove incorrect assumption @ 2015-10-03 12:44 Ray Donnelly 2015-10-03 15:38 ` Ray Donnelly 2015-10-03 17:13 ` Junio C Hamano 0 siblings, 2 replies; 9+ messages in thread From: Ray Donnelly @ 2015-10-03 12:44 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Ray Donnelly In normalize_ceiling_entry(), we test that normalized paths end with slash, *unless* the path to be normalized was already the root directory. However, normalize_path_copy() does not even enforce this condition. Even worse: on Windows, the root directory gets translated into a Windows directory by the Bash before being passed to `git.exe` (or `test-path-utils.exe`), which means that we cannot even know whether the path that was passed to us was the root directory to begin with. This issue has already caused endless hours of trying to "fix" the MSYS2 runtime, only to break other things due to MSYS2 ensuring that the converted path maintains the same state as the input path with respect to any final '/'. So let's just forget about this test. It is non-essential to Git's operation, anyway. Ack-by: Johannes Schindelin <johannes.schindelin@gmx•de> Signed-off-by: Ray Donnelly <mingw.android@gmail•com> --- test-path-utils.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/test-path-utils.c b/test-path-utils.c index 3dd3744..c67bf65 100644 --- a/test-path-utils.c +++ b/test-path-utils.c @@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct string_list_item *item, void *unused) if (normalize_path_copy(buf, ceil) < 0) die("Path \"%s\" could not be normalized", ceil); len = strlen(buf); - if (len > 1 && buf[len-1] == '/') - die("Normalized path \"%s\" ended with slash", buf); free(item->string); item->string = xstrdup(buf); return 1; -- 2.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-03 12:44 [PATCH 1/2] test-path-utils.c: remove incorrect assumption Ray Donnelly @ 2015-10-03 15:38 ` Ray Donnelly 2015-10-03 17:13 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Ray Donnelly @ 2015-10-03 15:38 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Ray Donnelly [-- Attachment #1: Type: text/plain, Size: 1731 bytes --] I'm going to have to attach this as a file, git-send-email isn't working for me; apologies. On Sat, Oct 3, 2015 at 1:44 PM, Ray Donnelly <mingw.android@gmail•com> wrote: > In normalize_ceiling_entry(), we test that normalized paths end with > slash, *unless* the path to be normalized was already the root > directory. > > However, normalize_path_copy() does not even enforce this condition. > > Even worse: on Windows, the root directory gets translated into a > Windows directory by the Bash before being passed to `git.exe` (or > `test-path-utils.exe`), which means that we cannot even know whether > the path that was passed to us was the root directory to begin with. > > This issue has already caused endless hours of trying to "fix" the > MSYS2 runtime, only to break other things due to MSYS2 ensuring that > the converted path maintains the same state as the input path with > respect to any final '/'. > > So let's just forget about this test. It is non-essential to Git's > operation, anyway. > > Ack-by: Johannes Schindelin <johannes.schindelin@gmx•de> > Signed-off-by: Ray Donnelly <mingw.android@gmail•com> > --- > test-path-utils.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/test-path-utils.c b/test-path-utils.c > index 3dd3744..c67bf65 100644 > --- a/test-path-utils.c > +++ b/test-path-utils.c > @@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct > string_list_item *item, void *unused) > if (normalize_path_copy(buf, ceil) < 0) > die("Path \"%s\" could not be normalized", ceil); > len = strlen(buf); > - if (len > 1 && buf[len-1] == '/') > - die("Normalized path \"%s\" ended with slash", buf); > free(item->string); > item->string = xstrdup(buf); > return 1; > -- > 2.5.2 [-- Attachment #2: 0001-test-path-utils.c-remove-incorrect-assumption.patch --] [-- Type: application/octet-stream, Size: 1710 bytes --] From 3deea12dd8506f88fdaeabcc33683f81b75a13fa Mon Sep 17 00:00:00 2001 From: Ray Donnelly <mingw.android@gmail•com> Date: Thu, 1 Oct 2015 20:04:17 +0100 Subject: [PATCH 1/2] test-path-utils.c: remove incorrect assumption In normalize_ceiling_entry(), we test that normalized paths end with slash, *unless* the path to be normalized was already the root directory. However, normalize_path_copy() does not even enforce this condition. Even worse: on Windows, the root directory gets translated into a Windows directory by the Bash before being passed to `git.exe` (or `test-path-utils.exe`), which means that we cannot even know whether the path that was passed to us was the root directory to begin with. This issue has already caused endless hours of trying to "fix" the MSYS2 runtime, only to break other things due to MSYS2 ensuring that the converted path maintains the same state as the input path with respect to any final '/'. So let's just forget about this test. It is non-essential to Git's operation, anyway. Ack-by: Johannes Schindelin <johannes.schindelin@gmx•de> Signed-off-by: Ray Donnelly <mingw.android@gmail•com> --- test-path-utils.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/test-path-utils.c b/test-path-utils.c index 3dd3744..c67bf65 100644 --- a/test-path-utils.c +++ b/test-path-utils.c @@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct string_list_item *item, void *unused) if (normalize_path_copy(buf, ceil) < 0) die("Path \"%s\" could not be normalized", ceil); len = strlen(buf); - if (len > 1 && buf[len-1] == '/') - die("Normalized path \"%s\" ended with slash", buf); free(item->string); item->string = xstrdup(buf); return 1; -- 2.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-03 12:44 [PATCH 1/2] test-path-utils.c: remove incorrect assumption Ray Donnelly 2015-10-03 15:38 ` Ray Donnelly @ 2015-10-03 17:13 ` Junio C Hamano 2015-10-04 14:51 ` Ray Donnelly 1 sibling, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2015-10-03 17:13 UTC (permalink / raw) To: Ray Donnelly; +Cc: git, Johannes Schindelin Ray Donnelly <mingw.android@gmail•com> writes: > In normalize_ceiling_entry(), we test that normalized paths end with > slash, *unless* the path to be normalized was already the root > directory. > > However, normalize_path_copy() does not even enforce this condition. Perhaps the real issue to be addressed is the above, and your patch is killing a coalmine canary? Some callers of this function in real code (i.e. not the one you are removing the check) do seem to depend on that condition, e.g. the codepath in clone that leads to add_to_alternates_file() wants to make sure it does not add an duplicate, so it may end up not noticing /foo/bar and /foo/bar/ are the same thing, no? There may be others. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-03 17:13 ` Junio C Hamano @ 2015-10-04 14:51 ` Ray Donnelly 2015-10-04 17:21 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Ray Donnelly @ 2015-10-04 14:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Sat, Oct 3, 2015 at 6:13 PM, Junio C Hamano <gitster@pobox•com> wrote: > Ray Donnelly <mingw.android@gmail•com> writes: > >> In normalize_ceiling_entry(), we test that normalized paths end with >> slash, *unless* the path to be normalized was already the root >> directory. >> >> However, normalize_path_copy() does not even enforce this condition. > > Perhaps the real issue to be addressed is the above, and your patch > is killing a coalmine canary? > > Some callers of this function in real code (i.e. not the one you are > removing the check) do seem to depend on that condition, e.g. the > codepath in clone that leads to add_to_alternates_file() wants to > make sure it does not add an duplicate, so it may end up not noticing > /foo/bar and /foo/bar/ are the same thing, no? There may be others. > > Enforcing that normalize_path_copy() removes any trailing '/' (apart from the root directory) breaks other things that assume it doesn't mess with trailing '/'s, for example filtering in ls-tree. Any suggestions for what to do about this? Would a flag be appropriate as to whether to do this part or not? Though I'll admit I don't like the idea of adding flags to modify the behavior of something that's meant to "normalize" something. Alternatively, I could go through all the breakages and try to fix them up? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-04 14:51 ` Ray Donnelly @ 2015-10-04 17:21 ` Junio C Hamano 2015-10-04 23:36 ` Ray Donnelly 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2015-10-04 17:21 UTC (permalink / raw) To: Ray Donnelly; +Cc: git, Johannes Schindelin Ray Donnelly <mingw.android@gmail•com> writes: >> Some callers of this function in real code (i.e. not the one you are >> removing the check) do seem to depend on that condition, e.g. the >> codepath in clone that leads to add_to_alternates_file() wants to >> make sure it does not add an duplicate, so it may end up not noticing >> /foo/bar and /foo/bar/ are the same thing, no? There may be others. > > Enforcing that normalize_path_copy() removes any trailing '/' (apart > from the root directory) breaks other things that assume it doesn't > mess with trailing '/'s, for example filtering in ls-tree. Any > suggestions for what to do about this? Would a flag be appropriate as > to whether to do this part or not? Though I'll admit I don't like the > idea of adding flags to modify the behavior of something that's meant > to "normalize" something. Alternatively, I could go through all the > breakages and try to fix them up? I agree with you that "normalize" should "normalize". Making sure that all the callers expect the same kind of normalization would be a lot of work but I do think that is the best approach in the long run. Thanks for the ls-tree example, by the way, did you find it by code inspection? I do not think it is reasonable to expect the test coverage for this to be 100%, so the "try to fix them up" would have to involve a lot of manual work both in fixing and reviewing, unfortunately. The first step of the "best approach" would be to make a note on normalize_path_copy() by adding a NEEDSWORK: comment to describe the situation. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-04 17:21 ` Junio C Hamano @ 2015-10-04 23:36 ` Ray Donnelly 2015-10-08 20:42 ` Ray Donnelly 0 siblings, 1 reply; 9+ messages in thread From: Ray Donnelly @ 2015-10-04 23:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Sun, Oct 4, 2015 at 6:21 PM, Junio C Hamano <gitster@pobox•com> wrote: > Ray Donnelly <mingw.android@gmail•com> writes: > >>> Some callers of this function in real code (i.e. not the one you are >>> removing the check) do seem to depend on that condition, e.g. the >>> codepath in clone that leads to add_to_alternates_file() wants to >>> make sure it does not add an duplicate, so it may end up not noticing >>> /foo/bar and /foo/bar/ are the same thing, no? There may be others. >> >> Enforcing that normalize_path_copy() removes any trailing '/' (apart >> from the root directory) breaks other things that assume it doesn't >> mess with trailing '/'s, for example filtering in ls-tree. Any >> suggestions for what to do about this? Would a flag be appropriate as >> to whether to do this part or not? Though I'll admit I don't like the >> idea of adding flags to modify the behavior of something that's meant >> to "normalize" something. Alternatively, I could go through all the >> breakages and try to fix them up? > > I agree with you that "normalize" should "normalize". Making sure > that all the callers expect the same kind of normalization would be > a lot of work but I do think that is the best approach in the long > run. Thanks for the ls-tree example, by the way, did you find it by > code inspection? I do not think it is reasonable to expect the test > coverage for this to be 100%, so the "try to fix them up" would have > to involve a lot of manual work both in fixing and reviewing, > unfortunately. For the ls-tree failure, I ran "make test" to see how much fell out. I'm not familiar with the code-base yet, so I figured that at least investigating the changes needed to make the test-suite pass would be a good entry point to reading the code; I will study it at the same time to try and get my bearings. > > The first step of the "best approach" would be to make a note on > normalize_path_copy() by adding a NEEDSWORK: comment to describe the > situation. > > Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-04 23:36 ` Ray Donnelly @ 2015-10-08 20:42 ` Ray Donnelly 2015-10-09 1:05 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Ray Donnelly @ 2015-10-08 20:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 2188 bytes --] On Mon, Oct 5, 2015 at 12:36 AM, Ray Donnelly <mingw.android@gmail•com> wrote: > On Sun, Oct 4, 2015 at 6:21 PM, Junio C Hamano <gitster@pobox•com> wrote: >> Ray Donnelly <mingw.android@gmail•com> writes: >> >>>> Some callers of this function in real code (i.e. not the one you are >>>> removing the check) do seem to depend on that condition, e.g. the >>>> codepath in clone that leads to add_to_alternates_file() wants to >>>> make sure it does not add an duplicate, so it may end up not noticing >>>> /foo/bar and /foo/bar/ are the same thing, no? There may be others. >>> >>> Enforcing that normalize_path_copy() removes any trailing '/' (apart >>> from the root directory) breaks other things that assume it doesn't >>> mess with trailing '/'s, for example filtering in ls-tree. Any >>> suggestions for what to do about this? Would a flag be appropriate as >>> to whether to do this part or not? Though I'll admit I don't like the >>> idea of adding flags to modify the behavior of something that's meant >>> to "normalize" something. Alternatively, I could go through all the >>> breakages and try to fix them up? >> >> I agree with you that "normalize" should "normalize". Making sure >> that all the callers expect the same kind of normalization would be >> a lot of work but I do think that is the best approach in the long >> run. Thanks for the ls-tree example, by the way, did you find it by >> code inspection? I do not think it is reasonable to expect the test >> coverage for this to be 100%, so the "try to fix them up" would have >> to involve a lot of manual work both in fixing and reviewing, >> unfortunately. > > For the ls-tree failure, I ran "make test" to see how much fell out. > I'm not familiar with the code-base yet, so I figured that at least > investigating the changes needed to make the test-suite pass would be > a good entry point to reading the code; I will study it at the same > time to try and get my bearings. > >> >> The first step of the "best approach" would be to make a note on >> normalize_path_copy() by adding a NEEDSWORK: comment to describe the >> situation. I hope this is acceptable for the first part of this task. >> >> Thanks. [-- Attachment #2: 0001-normalize_path_copy-NEEDSWORK-for-trailing.patch --] [-- Type: application/octet-stream, Size: 1110 bytes --] From e3f073b1e154f44c1e1c7716a2ed6977dd144224 Mon Sep 17 00:00:00 2001 From: Ray Donnelly <mingw.android@gmail•com> Date: Thu, 8 Oct 2015 17:10:28 +0100 Subject: [PATCH 1/2] normalize_path_copy: NEEDSWORK for trailing '/' Currently, normalize_path_copy () does nothing regarding trailing '/'s. Instead it should remove them except in the case of the root folder. --- path.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/path.c b/path.c index 65beb2d..4ae6db6 100644 --- a/path.c +++ b/path.c @@ -893,6 +893,11 @@ const char *remove_leading_path(const char *in, const char *prefix) * normalized, any time "../" eats up to the prefix_len part, * prefix_len is reduced. In the end prefix_len is the remaining * prefix that has not been overridden by user pathspec. + * + * NEEDSWORK: This function doesn't perform normalization w.r.t. trailing '/'. + * For everything but the root folder itself, the normalized path should not + * end with a '/', then the callers need to be fixed up accordingly. + * */ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len) { -- 2.6.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-08 20:42 ` Ray Donnelly @ 2015-10-09 1:05 ` Junio C Hamano 2015-10-09 10:12 ` Ray Donnelly 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2015-10-09 1:05 UTC (permalink / raw) To: Ray Donnelly; +Cc: git, Johannes Schindelin I'll squash this in as part of your first patch that removes the test from test-path-utils.c. That makes it clearer why it is the right thing to remove the test, I'd think. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption 2015-10-09 1:05 ` Junio C Hamano @ 2015-10-09 10:12 ` Ray Donnelly 0 siblings, 0 replies; 9+ messages in thread From: Ray Donnelly @ 2015-10-09 10:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Fri, Oct 9, 2015 at 2:05 AM, Junio C Hamano <gitster@pobox•com> wrote: > I'll squash this in as part of your first patch that removes the > test from test-path-utils.c. That makes it clearer why it is the > right thing to remove the test, I'd think. > Great, many thanks! > Thanks. > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-09 10:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-03 12:44 [PATCH 1/2] test-path-utils.c: remove incorrect assumption Ray Donnelly 2015-10-03 15:38 ` Ray Donnelly 2015-10-03 17:13 ` Junio C Hamano 2015-10-04 14:51 ` Ray Donnelly 2015-10-04 17:21 ` Junio C Hamano 2015-10-04 23:36 ` Ray Donnelly 2015-10-08 20:42 ` Ray Donnelly 2015-10-09 1:05 ` Junio C Hamano 2015-10-09 10:12 ` Ray Donnelly
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox