-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Attempting to handle case insensitive case. #64699
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #64751) made this pull request unmergeable. Please resolve the merge conflicts. |
I made a try at this here. The problem is a bit more complicated than it might look like. :) |
Haha, yeah I figured 100%. Mainly because of the fact that old URL links would possibly break with a new generation. Are there other things beyond that? Just curious what other difficulties there are. |
No, it was mostly trying to do it without adding too much burden. However, I didn't (and still don't...) have enough time to implement fully what I have in mind. |
…here, r=<try> No StableHasherResult everywhere This removes the generic parameter on `StableHasher`, instead moving it to the call to `finish`. This has the side-effect of making all `HashStable` impls nicer, since we no longer need the verbose `<W: StableHasherResult>` that previously existed -- often forcing line wrapping. This is done for two reasons: * we should avoid false "generic" dependency on the result of StableHasher * we don't need to codegen two/three copies of all the HashStable impls when they're transitively used to produce a fingerprint, u64, or u128. I haven't measured, but this might actually make our artifacts somewhat smaller too. * Easier to understand/read/write code -- the result of the stable hasher is irrelevant when writing a hash impl. cc @eddyb @nikomatsakis I guess? Not sure whose purview this falls under r? @michaelwoerister due to touching incremental
… needs to take that into account.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
if you don't mind, can I play around with this pull request more? Don't want to annoy you while I push stuff is why I'm asking. |
No problem, go ahead! Maybe you'll find a better solution than mine. :) |
This is an attempt at dealing with #25879.
Basically ensure_dir and item_path both now use a map to identify what path to use where it's like
If the item isn't in the map, then we can use the original location. If something already points to that location, I append a number based on how many other items are in the map to the folder name or item name, store that in the map, and then return said path / item name. ensure_dir gained a sibling function called ensure_dir_lower because if a path overlaps but it's technically a different module, we need to give a new destination to use.
So as an example of how this works, if we're seeing the situation
Then the first one is entered as struct.FoO.html. The second struct Foo would be considered overlapping since they share the same lowered version, and its url becomes struct.foo.1.html. This works with the modules too, so if two modules had the same names as the structs above, we'd see a folder created for foo and foo.1
Things I have questions about:
Is there a better naming method to use? I know Windows generally does things as " (1)", but don't know what would be appropriate for the web.
Should I potentially be using different variables for things like cached_dir?
I had to wrap cached_dir with a mutex because I wanted to make sure we didn't run into a situation with parallelization where two overlapping paths check and see if the folder path had been created, see it hasn't, and then both create the same path. But that mutex would now affect the case sensitive path too. Is there a better way of locking just the lowered version of the function, or maybe another way of handling that situation in general?
The way I have to build the new url in
fn append
feels pretty inefficient, and didn't know if there was a better way of handling it.Also definitely need to update that option to not always be true.