[PATCH 1/2] test-path-utils.c: remove incorrect assumption

[PATCH 1/2] test-path-utils.c: remove incorrect assumption

[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

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[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

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Junio C Hamano]

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.

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Ray Donnelly]

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?

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Junio C Hamano]

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.

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Ray Donnelly]

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.

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Ray Donnelly]

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

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Junio C Hamano]

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.

Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption

[Ray Donnelly]

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