`set PATH arg:with:colons` prints superfluous error

[lilyball]

In Fish 3.2 and earlier, the following works just fine:

set PATH /usr/local/bin:/usr/bin

This splits the input on colons and sets $PATH to the result.

In Fish 3.3.0, this is broken. The above code will result in a warning and not actually change the value of $PATH at all:

set: Warning: $PATH entry "/usr/local/bin:/usr/bin" is not valid (No such file or directory)
set: Did you mean 'set PATH $PATH /usr/bin'?

The colon-splitting behavior still works just fine with other path variables (such as $MANPATH or variables defined using set --path), but it simply won't work with PATH even if I pass the --path flag.

This is especially problematic for code that sets variables without knowing what they are. For example, this triggers on code of mine that looks like

set -xg (string split -m 1 = $line)

[lilyball]

It does this with CDPATH too.

[lilyball]

Interesting quirk: If I run the following:

set PATH /usr/local/bin /usr/bin:/bin

I get the warning

set: Warning: $PATH entry "/usr/bin:/bin" is not valid (No such file or directory)
set: Did you mean 'set PATH $PATH /bin'?

But it also sets the path variable correctly!

$PATH: set in global scope, exported, a path variable with 3 elements
$PATH[1]: |/usr/local/bin|
$PATH[2]: |/usr/bin|
$PATH[3]: |/bin|

I think it's doing incorrect validation of PATH and CDPATH.

That said, in Fish 3.2.2 I can't get it to show this warning at all. So perhaps the validation was broken before, and fixed in 3.3.0, without realizing that it broke the ability to actually write set PATH /usr/local/bin:/usr/bin:/bin.

[lilyball]

Well, I say "fixed", but the validation looks like it's only detecting entries that "refer to invalid directories that contain a colon". Which means it's not actually detecting invalid directories in general, just when a colon is involved. Which also means it's completely ignoring the fact that the values will subsequently be split on colons once it gets to the env layer and therefore saying "the directory 'foo:bar' doesn't exist" is pointless as the variable will never actually contain the entry foo:bar.

Which is to say, the following runs without error:

> mkdir foo:bar
> set PATH /usr/local/bin /usr/bin $PWD/foo:bar
> set -S PATH
$PATH: set in global scope, exported, a path variable with 4 elements
$PATH[1]: |/usr/local/bin|
$PATH[2]: |/usr/bin|
$PATH[3]: |/Users/lily/foo|
$PATH[4]: |bar|

Note how the resulting PATH does not in fact include /Users/lily/foo:bar as an entry.

I'm guessing this validation is the vestigial leftovers of the old validation that warned if you added any entry to PATH that didn't exist. My best guess at intent here is "if you say set PATH /usr/local/bin /foo /bar we'll allow it, but if you say set PATH /usr/local/bin:/foo:/bar we'll reject the invalid dirs", except it's not colon-splitting the arg and testing the components.

Also interesting is how set PATH /foo /bar exits with a status of 1 without printing anything at all (assuming /foo and /bar don't exist) but set PATH /usr/local/bin /foo /bar succeeds. The idea of "don't set the PATH to something that contains no valid dirs" is reasonable, but where's the error message? Again I think this is the outcome of neutering the old "you can't add any dirs that don't exist" logic.

[faho]

Yeah, the validation is borken and should probably just be removed entirely. We used to print an error for invalid paths, this seems to be a remnant of that.

Feel free to commit a fix, you still have the bit.

[lilyball]

I think I do, but personal circumstances mean I can’t contribute code at the moment.