Accept ToComponents in gix_fs::Stack

[cruessler]

This is a draft PR, intended to make sure I got the task right before making any larger changes that I would need to make now that I’ve changed gix_fs::Stack’s API. It is expected that it does not even compile at this stage.

A few observations:

• There’s places that currently call Stack::new with a PathBuf. Would it make sense to impl ToComponents for PathBuf?
• Having to add bstr as a dependency made me wonder: is gix-fs the right crate for ToComponents to live in or should it be moved somewhere else, e. g. into gix-path? gix-path also has os_string_into_bstring which should probably replace the chain .as_os_str().as_bytes().into() that I had to resort to on one line because gix-fs does not depend on gix-path currently.

Let me know what you think!

[Byron]

Thanks for getting started on this, it's much appreciated!

• There’s places that currently call Stack::new with a PathBuf. Would it make sense to impl ToComponents for PathBuf?

No - I have made some more comments in code with details.

• Having to add bstr as a dependency made me wonder: is gix-fs the right crate for ToComponents to live in or should it be moved somewhere else, e. g. into gix-path? gix-path also has os_string_into_bstring which should probably replace the chain .as_os_str().as_bytes().into() that I had to resort to on one line because gix-fs does not depend on gix-path currently.

Indeed, I think gix-path is the better spot for it. The reason is also that I think it will harbour the RepositoryPathBuf/RepositoryPath one day, which will be the typed version of a BString as we use it today.

The place where ToComponents needs to go is make_relative_path_current() and its callers. Then the callers have to be adjusted to avoid calls to gix::path::<conversion> as these shouldn't be needed anymore.

I hope that helps.

[Byron]

These have to stay PathBuf as the type is a link between relative paths and the filesystem location they represent.
It's also in the interest of performance to keep it close to what it ought to represent.

[Byron]

Since this is the root and it is a filesystem location, it must remain a PathBuf.

ToComponents is really only for the relative paths that are passed in.
It's interesting that ToComponents currently doesn't convey information about whether the component is coming from a relative path, which kind of is a problem, but maybe one that can be tackled later.

[cruessler]

I’m running into an issue when trying to turn relative: &Path into relative: impl gix_path::ToComponents. The !matches!(comp, Component::Normal(_)) in the code below does not work anymore when omponents is Vec<&BStr> instead of Components<'_>. It seems we would already need something like RepositoryPath for that matches! to work properly. Or am I missing anything? Removing the check does not seem to be an option as it would alter the function’s behaviour.

gitoxide/gix-fs/src/stack.rs

Lines 103 to 111 in c8c42b4

	if !matches!(comp, Component::Normal(_)) {
	return Err(std::io::Error::new(
	std::io::ErrorKind::Other,
	format!(
	"Input path \"{}\" contains relative or absolute components",
	relative.display()
	),
	));
	}

[Byron]

I think the problem here is twofold:

• the Stack wants to (and has to) build an actual Path, so each component of the iterator needs to be an OsStr`, which is fallible at least on Windows.
• The conversion (and component) check could be put into the iterator itself (especially if it is fallible)

To me this means the trait would have to be owned by gix-fs for now as it needs to assure there are no non-normal components. Maybe that would be a start?

Another conclusion here probably is that there will be many ToComponents implementations, with encoding different requirements, and that's probably fine as well. We might consider naming them accordingly, this one here could be a ToPathComponent for instance.

[cruessler]

I hope what I just pushed is closer to your suggestions, even though the new trait ToPathComponents lives in gix-path instead of gix-fs. So far, I’ve tried to change as little as possible of the existing code. That’s the reason I’ve added .collect() because that way .is_empty() can still be used.

[Byron]

Thanks a lot - this looks like it should work!
Something I don't understand is how the MSRV check can fail while ToComponents in gix does the very same - it is using a feature that ought to be available from Rust 1.75, just one version above the current MSRV which has to be held until helix upgrades their MSRV. Let's check :).
As a next step, all the callers of make_relative_path_current() would have to be adjusted to use the trait too - in theory this won't be a breaking change as the trait will then be extended to be more compatible.
Please feel free to merge my commit down into yours and adjust the subject lines accordingly, as I changed names once again 😅. Oh, and it felt much better to keep ToNormalPathComponents where it is used and tested, so I moved it. This means the previous commits now would need to be reworked for sure.

@pascalkuthe Would helix be able to update their MSRV to something newer?

[pascalkuthe]

I didn't quite get which MSRV version you are looking for. Is it 1.75? That would be fine for us.

[Byron]

Yes, that’s the one. Great!

[cruessler]

I’ve rebased and merged the commits, so now there’s only 2 left which definitely is a simplification. Before I randomly start changing callers, up to which level do we want to accept relative_path: ToNormalPathComponents?

[Byron]

A good point! There shouldn't be too many to begin with, and it looks like there are 3 in production code.

I could probably take it from here if you wanted me to.

[cruessler]

Feel free to take over! I’ve just marked this PR as ready and will have a look at the next item on your list of suggestions!