Add internal C++ native path manipulation functions #3442
Conversation
|
Oh and FWIW, these are intentionally not returning filesystem::path items as those have some peculiar charasteristics that don't play well with our codebase. Like appending an absolute path to an existing path resetting the whole path to the part that was supposed to be appended, ie Also, the placement to rpmmacro_internal.hh is a bit meh, but the places that need this are likely to include that anyhow and adding a new header just for these seems silly so... Dunno, other opinions are welcome certainly. (and this really belongs to the commit message adding these functions, I'll fixup along with other review feedback when it comes) |
|
Is there a path (pardon the pun) to us being able to adopt |
|
No, other than "use it where it makes sense". Quite apparently it doesn't in all sorts of scenarios. |
|
The pun was pretty funny though 😄 |
| @@ -306,79 +307,34 @@ char *rpmCleanPath(char * path) | |||
| return path; | |||
| } | |||
|
|
|||
| /* Merge 3 args into path, any or all of which may be a url. */ | |||
| /* Merge 3 args into path */ | |||
There was a problem hiding this comment.
Did we drop support for URLs?
There was a problem hiding this comment.
Not really, the URL support never worked at all (as is explained in the commit message and the API docs update). Any URL's in there and the output is utterly useless garbage.
There was a problem hiding this comment.
So it was a lucky case of being SO broken nobody could have possibly used it for URL's, so we can just prune that cruft.
There was a problem hiding this comment.
So I guess stuff like rpm -ivh https://kojipkgs.fedoraproject.org//packages/rpm/4.20.0/1.fc42/src/rpm-4.20.0-1.fc42.src.rpm are handled elsewhere?
It's 2024, isblank() is available. Only, we prefer to use our own versions of these things. I don't know that this is any less undefined behavior than the standard versions but it's what we use elsewhere and works for the purpose so shrug.
Add C++ native counterparts for rpmGenPath(), rpmGetPath() and rpmCleanPath(), but with extended functionality and hopefully more obvious names. rpmmacro_internal.hh is perhaps a bit strange place to put these but creating a new header just for these also seems like an overkill. - rpm::expand_path() catenates its arguments into a string that is then macro expanded and the resulting path normalized. Ie same as rpmGetPath(). - rpm::join_path() joins arbitrary number of arguments into a path, optionally macro expanding each argument before joining. Resulting path is normalized. Ie same as rpmGenPath() but with flexible number of arguments. - rpm::normalize_path() normalizes a path, ie remove double slashes and other excess stuff. With the exception of rpmCleanPath(), make the C version call the C++ counterpart. rpmCleanPath() expects to modify its argument in any number of unsafe ways, there's no saving it. rpmGenPath() API docs claimed to handle URL's in various fancy ways but that stuff was so broken nobody could have possibly used it, so simply remove all that useless URL crap from there. Note that we're intentionally not using std::filesystem::path here at least for now, the path append semantics are at odds with rpm usage, for example we commonly have intermediate paths starting with a slash and with filesystem::path, "aa" / "/bb" equals "/bb" whereas we expect "aa/bb" in such a case. Fow now, a caller can always just assign into an STL ::path if that's what they want.
No functional changes.
|
The one debug leftover dropped and more elaborate commit message on the main commit (about why filesystem::path is not used etc) |
|
Also FWIW I'm totally okay with splitting the latter commits to another PR if this seems a bit much at a time. I wanted to convert some uses to the new native versions directly, but when getting rid of rpmCleanPath() turned out to be a reachable target I got carried away a bit 😆 |
|
Having slept on it, indeed backing off a bit: the test-coverage on fingerprinting and related query stuff is sketchy at best, I'll sleep better after ensuring proper coverage before changing those. I want to get rid of rpmCleanPath() but need a little more patience there. |
The interesting part of this PR are new rpm::join_path(), rpm::expand_path() and rpm::normalize_path() functions which are more powerful C++ native counterparts of rpmGenPath(), rpmGetPath() and rpmCleanPath(), and hopefully with more meaningful names too.
rpmGenPath() and rpmGetPath() use the C++ versions internally now, which "proves" they work as promised. rpmCleanPath() can't do so because it expects to manipulate the C string buffer passed to it. We'll deprecate it as soon as we get rid of it.
The latter commits replace a few uses of rpmCleanPath() and related C-side functions with C++ native versions to further prove these work as intended, but technically they wouldn't need to be in this PR.