Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption
On Mon, Oct 5, 2015 at 12:36 AM, Ray Donnelly <mingw.android@gmail.com> wrote:
Show 37 quoted lines
> 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.
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
Ray Donnelly <mingw.android@gmail.com>