[ospaths] fix #8491: XDG_CONFIG_HOME was being ignored due to #8681 + private normalizePathEnd

[timotheecour]

• fixes XDG_CONFIG_DIR env var ignored (prevents disabling ~/.config/nim.cfg for 1 process) #8491 (which is actually still broken: XDG_CONFIG_HOME was being happily ignored due to a bug in nim:when when Foo wrongly accepts non-bool Foo #8681), see [1]
• adds (private) normalizePathEnd which is needed here and in other upcoming PR's to ensure we have exactly 0 or 1 trailing dir separators (taking care of special cases on windows); the previous coded didn't guarantee that (on top of the main bug)

[1] here's the bug: XDG_CONFIG_HOME is ignored:
XDG_CONFIG_HOME=FOO rnim bugs/compiler/t20_XDG_CONFIG_HOME.nim
nim c --nimcache:/tmp/nim//nimcache/ -o:/tmp/nim//app -r bugs/compiler/t20_XDG_CONFIG_HOME.nim
getConfigDir()=/Users/timothee/.config/

bugs/compiler/t20_XDG_CONFIG_HOME.nim:

import ospaths
echo "getConfigDir()=", getConfigDir()

[timotheecour]

/cc @Varriount I'd love to get this merged ASAP as it fixes an annoying bug; thanks!

[Araq]

• What is the remaining bug? (Yes, I can guess it, but you didn't say.)
• Why do you rewrite working code in your preferred style?
• Why do you conflate a bugfix with a new feature? There is no reason normalizePathEnd cannot stay private.
• Only the path using XDG_CONFIG_HOME has the bug, but your code unconditionally calls normalizePathEnd which probably allocates.
• The better design seems to me to make getConfigDir not return a string that ends in /. People should write getConfigDir() / "x", not getConfigDir() & x.

[Araq]

/cc @Varriount I'd love to get this merged ASAP as it fixes an annoying bug; thanks!

I can see what you did here and I'm mildly offended. ;-)

[timotheecour]

What is the remaining bug? (Yes, I can guess it, but you didn't say.)

well i mentioned "#8491 XDG_CONFIG_HOME was being happily ignored"; but ok, I just added test case to show the bug in top-level message

Why do you rewrite working code in your preferred style?

the original code had style issues, eg using & "/.config/" instead of the idiomatic / ; furthermore it had other issues (eg ending up with multiple trailing /) beyond the bug being fixed.

But ok, I just added a commit to remove some of the excessive UFCS.

Why do you conflate a bugfix with a new feature? There is no reason normalizePathEnd cannot stay private.

ok; made it private for now.

Only the path using XDG_CONFIG_HOME has the bug, but your code unconditionally calls normalizePathEnd which probably allocates.

well the other issue was that old code could end up with double trailing DirSep (in both windows and posix); after the PR, this is fixed.
Wouldn't & "\\" also allocate? in any case, being correct should come before being fast; and we can always optimize out un-necessary allocations in normalizePathEnd later (it'll be just in 1 function instead of spread out everywhere)

The better design seems to me to make getConfigDir not return a string that ends in /.

It's right there in the documentation: An OS-dependent trailing slash is always present at the end of the returned string
That would be a breaking change, which I don't wanna do in this PR.

People should write getConfigDir() / "x", not getConfigDir() & x.

I agree with that statement.
However, it's not incompatible with getConfigDir ending in / : once joinPath gets fixed to avoid inserting double / on joinPath("foo/", "bar"), this won't be a problem; Note that my other pending PR #8351 does fix that.

I can see what you did here and I'm mildly offended. ;-)

not sure I follow; keep in mind this is all volunteer work. Do you want me to CC you in every PR? I could do that; I assumed you may not want to review every single PR, which doesn't scale, and I'm assuming everyone with commit rights is expected to do a good review.

[Araq]

Take a look at my alternative implementation please.

[Araq]

$DirSep is better style.

[timotheecour]

done

[Araq]

I think more correct would be this check if result.len == 2 and result[1] == ':'

[timotheecour]

done

[Araq]

Again, space after colon.

[timotheecour]

done

[Araq]

This is still "meh". How about this:

proc normalizePathEnd(path: var string, trailingSep = false) =
  var i = path.len
  while i >= 1 and path[i-1] in {DirSep, AltSep}: dec(i)
  if trailingSep and i == path.len:
    path.add DirSep
  else:
    path.setLen(i+ord(trailingSep))

[timotheecour]

this had a few bugs:
normalizePathEnd should only normalize path end, and should not change whether a path is absolute or relative; the version above could turn "/" into "" or vice versa.

I added test cases to make sure we prevent future regressions) but I reimplemented starting from your version and trying hard to minimize branching.

[Araq]

It's not so much about minimzing branching, it's about avoiding stdlib procs that never quite do the right thing and you are left with the hairy special cases anyway.

[Araq]

It's right there in the documentation: An OS-dependent trailing slash is always present at the end of the returned string
That would be a breaking change, which I don't wanna do in this PR.

Ok, fair enough, the other get*Dir() procs also seem to produce trailing slashes... (Bah!)

[timotheecour]

thx, will fix all these this weekend

[dom96]

Wouldn't & "\" also allocate? in any case, being correct should come before being fast

As far as correctness is concerned, isn't a path like C:\foo\\file.txt\\ correct on Windows? Similar for /home/dom//file.txt//?

By "correct" I mean "it works, the OS doesn't care that the slashes are doubled".

[Araq]

By "correct" I mean "it works, the OS doesn't care that the slashes are doubled".

Pretty sure this is the case, however allowing these makes path comparisons much trickier to implement correctly.

[timotheecour]

By "correct" I mean "it works, the OS doesn't care that the slashes are doubled".

Pretty sure this is the case, however allowing these makes path comparisons much trickier to implement correctly.

and there are existing bugs related to that: eg just filed #8734 (yes, these will be eventually fixed). But Nim should not introduce new double slashes (in getConfigDir, before this PR, it could introduce double slashes even though XDG_CONFIG_HOME didn't contain double slashes)

[timotheecour]

@Araq PTAL

[Araq]

'normalizePathEnd' is still the wrong idea.

cmpPaths("C://foo/bar", r"C:\foo\bar") --> should be equal
probably it's better to push the complexity into the cmpPaths logic instead of trying to "normalize" every single path in the APIs (which would also be slower).

[timotheecour]

cmpPaths("C://foo/bar", r"C:\foo\bar") --> should be equal

I just filed #8780 for that

yes, cmpPaths should do the right thing (and #8780 will be fixed eventually); not arguing with that.

What I'm arguing for is that our API's should not introduce new style violation (by that I mean duplicated DirSep):
before this PR, getConfigDir can return a style violation even though XDG_CONFIG_HOME doesn't have style violation
style issues affect both Nim API's (see #8734 and #8780 etc) and non-Nim APIs (over which we have no control)

As shown in this PR, getting the details right is tricky, so that's why this PR is needed so the tricky work is handled in just 1 place.
There's a few other places where normalizePathEnd can be used; note that, in its latest form, it wont' have any practical impact on runtime performance.

[timotheecour]

thanks