builddep: Escape glob characters in pkg specs #1088
Conversation
| out = std::regex_replace(out, std::regex("\\["), "\\["); | ||
| out = std::regex_replace(out, std::regex("\\]"), "\\]"); | ||
| out = std::regex_replace(out, std::regex("\\*"), "\\*"); | ||
| out = std::regex_replace(out, std::regex("\\?"), "\\?"); |
There was a problem hiding this comment.
Such strings are going to stay in the "glob pattern" category because of the simple check. Is that OK for is_glob_pattern callers?
There was a problem hiding this comment.
I assume yes... just that DNF5/libdnf goes through more expensive code paths... but long-term, #1085 looks like semantically more correct. Perhaps "the" issue is about to be fixed in two phases?
There was a problem hiding this comment.
Yes, the code path is a bit more expensive, but anyway I think the escaped strings need to be handled as globs or else the escaping backslashes (\[, etc) will be treated as literal and not as escaping the following glob character.
#1085 is definitely the better fix in the long term, but I think both patches are small and tidy enough that we can do this for now, and possibly revert it in favor of #1085 later.
There was a problem hiding this comment.
Regular expressions seem to be a bit of overkill here. How about doing the escaping in a single loop?
std::string out;
for (const auto ch : str) {
if (ch == '*' || ch == '?' || ch == '[' || ch == ']') {
out += '\\';
}
out += ch;
}
return out;
But I'm not blocking the merge on it if you prefer regex.
There was a problem hiding this comment.
Done, that's saner and I think still plenty readable.
| out = std::regex_replace(out, std::regex("\\["), "\\["); | ||
| out = std::regex_replace(out, std::regex("\\]"), "\\]"); | ||
| out = std::regex_replace(out, std::regex("\\*"), "\\*"); | ||
| out = std::regex_replace(out, std::regex("\\?"), "\\?"); |
There was a problem hiding this comment.
Regular expressions seem to be a bit of overkill here. How about doing the escaping in a single loop?
std::string out;
for (const auto ch : str) {
if (ch == '*' || ch == '?' || ch == '[' || ch == ']') {
out += '\\';
}
out += ch;
}
return out;
But I'm not blocking the merge on it if you prefer regex.
Not parsing the pkg specs as globs would require an ABI-breaking change (rpm-software-management#1085). A workaround that doesn't involve breaking changes is to escape the glob characters in the pkg specs. Related: rpm-software-management/mock#1267 Resolves rpm-software-management#1084
evan-goode
force-pushed
the
evan-goode/builddep-escape-glob
branch
from
2b7da7b to
78a462f
Compare
a4662a7
Not parsing the pkg specs as globs would require an ABI-breaking change (#1085). A workaround that doesn't involve breaking changes is to escape the glob characters in the pkg specs.
Related: rpm-software-management/mock#1267
Resolves #1084