Sound path

[mattsc]

Making this a PR only to make sure I am not missing something and CI passes.

[mattsc]

Well, and to make sure it does not include another commit, I guess ...
I'll fix that.

[mattsc]

Oh, I just noticed, the comments in #3777 point out other occurrences of this as well. I'll check on those later, don't have time right this moment.

[mattsc]

So ... I've now done this for file_system.cpp and config_cache.cpp, see a0a790b. This removes the actual user name from a lot of output, as can be seen if you run Wesnoth with --log-debug=filesystem or --log-debug=cache.

But ... With that, I am probably less than 20% done. Before I continue, I'd like some input as to whether this is actually worth it. And more so, whether we actually want to do this. It's not difficult work, but it is tedious, so I'd like to know whether it is worth continuing.

[Pentarctagon]

It seems like this might be something better done in the logger itself, rather than trying to track down every individual place that might log the current user? So like have the logger check every log statement for the text /<current user>/ and substitute it if found.

[mattsc]

Hmm, yeah, good point. I should have thought of that myself. There are some instances that do not go through the logger, but those are comparatively few and should be easy to take care of separately.

[mattsc]

Kicking myself a little here, but at least for the most part it seems to be as simple as 082d23a

Obviously I am going to clean all of this up later, for now I just want to see if this passes CI.

[Pentarctagon]

Does sanitize_path work as-is? Mainly wondering what happens when it calls normalize_path on something that's not a filesystem path string.

[mattsc]

Does sanitize_path work as-is? Mainly wondering what happens when it calls normalize_path on something that's not a filesystem path string.

It does work as is, it just does a string raplacement of the user name to "USER". If that user name does not appear, nothing happens. Except that it also "normalizes" the path, showing the separators the same on all systems in the output, independent of what the OS uses, if I understand correctly.

So all of that has two problems. If the user name is something that also appears in other log strings (such as, say, 'ai' or 'Konrad'), or if the separator might appear in an equation or operation, for example when the output is code.

Bottom line is, doing this by default for all strings in the logger is probably not a good idea. We either need to detect whether the message is a directory (I don't know if we can do that reliably) or do it manually for each log message using file names or paths after all.

[Pentarctagon]

I was thinking more that it would make more sense to just not use sanitize_path at all. Rather, something like:

void sanitize_log(std::string& logstr)
{
    #ifdef _WIN32
	    const char* user_name = getenv("USERNAME");
	    boost::replace_all(logstr, "\\"+user_name+"\\", "\\USER\\");
    #else
	    const char* user_name = getenv("USER");
	    boost::replace_all(logstr, "/"+user_name+"/", "/USER/");
    #endif
}

[mattsc]

So there's my next attempt. That seems to work, but I'm not sure about all that back and forth between string and const string and const char* and whatnot. Couldn't get it to work otherwise, but I am probably just missing something simple.

Oops -- and the extra string at the output needs to be deleted, of course. That's left over from me testing something early on.

[Pentarctagon]

Overall it looks fine to me. I do wonder if we need sanitize_path at all now.

[mattsc]

@Pentarctagon I think this should be ready now. I did the following:

• Removed the griiiiit (which made it pass unit tests)
• Renamed variables as you suggested
• Went through the code and added filesystem::sanitize_path to output that does not go through the logger. There is quite a bit of that, actually, incl. some that already used it and therefore does not show up in the commit.

[Pentarctagon]

I do also wonder why Wesnoth has places bypassing regular logging, but I can take a look at that after this is merged.

[mattsc]

I do also wonder why Wesnoth has places bypassing regular logging, but I can take a look at that after this is merged.

Yeah, that was my take on that.