@rnhmjoj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jgillich and @dezgeg to be potential reviewers.

Are there any other conditions under which fish set can output to stderr? I just want to make sure we don't end up suppressing other errors which we might actually care about. If there could be, we should actually check and only redirect stderr when we know the directories we're adding to the path don't currently exist. If not, I think this should be fine.

Fair point. Probably not but it could happen at some point in the future.

If we only want to suppress a specific error message, maybe we could find a fish-y way to do something like this and grep just for the error message we don't want. We could do this in NixOS without patching fish-foreign-env.

Patching foreign-env to only hide the path warnings looks quite ugly:

    if test "$key" = 'PATH'
      for i in (echo $value | tr ':' '\n')
        if test -d "$i"
          set -g -x PATH $PATH "$i"
        else
          set -g -x PATH $PATH "$i" ^/dev/null
        end
      end
    else
      set -g -x $key $value
    end

maybe we could find a fish-y way to do something like this

This goes beyond my (rather limited) understanding of shell scripting but looks like the best way.

I figured it out, and it's easier than I thought it would be!

Instead of fenv <somefile> ^ /dev/null, we can just do fenv <somefile> ^| grep -v 'set: Warning: $PATH entry ".*" is not valid (No such file or directory)'.

I'll submit a NixOS PR as soon as I get a chance to test it at home (I've tested a fake version using echo on this macOS machine). :-)

I would make it simple and just redirect exporting PATH to /dev/null:

if test "$key" = 'PATH'
  set -g -x PATH $PATH "$i" ^/dev/null
else
  set -g -x $key $value
end

It seems unlikely that future versions of fish would change the semantic of this simple expression.
Grepping the output on the other hand can break, if the message changes slightly. Also it adds more overhead due the additional grep call.

@Mic92 I implemented your suggestion.

The set of PATH entries added by the Nix packaged version is finite. So, all that needs to be done is that we need to keep track of which entries were automatically generated by Nix and filter those out. Given that the initial PR suggests that this is a tiny, tiny set a solution appears trivial. (Iterate over all entries in the PATH and only display a warning when the directory does not exist and it is in the set.)

Why do you come up with a solution which destroys the warning feature completely?

I also have a meta comment about this PR, which is that there is no design. There is just code. The code should be the implementation of some design. What I have written down is a high level design. In a PR where the first solution wasn't accepted, you should get the hint to level up your game.

That is, I would expect my high level design to be refined to a specific implementation design (e.g. that you want to implement it using code written in fish) and then that's something I could look at and judge in mere seconds.

Look in the mess in which you are now. You only spent time implementing a wrong solution twice during which you wasted the time of every reviewer involved as well as your own.

Please close this PR, open a new one taking into account that

• it has to work
• it needs to respect the semantics of fish (i.e. do not destroy the spirit of the fish warning feature
• it should not be terribly slow
• someone reading the code shouldn't be required to be a fish expert to know what's going on (e.g. I had to google some syntax, which would have been unnecessary if this had been written with the intention of being readable)

The set of PATH entries added by the Nix packaged version is finite. So, all that needs to be done is that we need to keep track of which entries were automatically generated by Nix and filter those out. Given that the initial PR suggests that this is a tiny, tiny set a solution appears trivial.

It does not appear trivial to me. I don't know how I can match against a set of strings generated by nixos (which is entirely configurable and may change from user to user) in a shell script outside nixos.

Why do you come up with a solution which destroys the warning feature completely?

It does not. It's hiding the warning when sourcing environment with fenv. The warning it's still there when you do set PATH [...]

Look in the mess in which you are now. You only spent time implementing a wrong solution twice during which you wasted the time of every reviewer involved as well as your own.

wow. Don't you think you are overstating things a bit here?

Please close this PR, open a new one taking into account that

Sorry, I'm not closing this PR. This is a valid solution, it's not terribly slow and it's only a minor change in behavior.

someone reading the code shouldn't be required to be a fish expert

I'm not a "fish expert" myself.

which would have been unnecessary if this had been written with the intention of being readable

Are you saying I'm terrible at writing code or that I'm just altogether evil?

@rnhmjoj I already had expected you would trip over the word "completely". Sure, in many cases it would work, but not in all.

Regarding the readability, it's not a matter of being evil, but more that you likely don't care about writing something which is good, rather than something which merely works in many cases.

I can assure you that the person who opened a pull request after you already has a better one, but I also rejected that one.

If everything we have proposed is terrible and merely works I'd like to see your proposal.
As code, in a PR: not a "high-level design".