patch, 2 partstest-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

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

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

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

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

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