GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

progress.rs misleadingly claims "we target 32bit systems only" #1530

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

Current behavior 😯

It's unclear why core.packedGitLimit is not applicable.

progress.rs gives the status of core.packedGitLimit as:

https://github.com/Byron/gitoxide/blob/242fedc973c56b6c1b6f150af99dda972a67f547/src/plumbing/progress.rs#L77-L80

This item was introduced in 0e9bd41 (#555).

I'm not clear on what "we target 32bit systems only" means here. It sounds like it is saying the code is not meant to be built and run on 64-bit targets, which is of course not the case--64-bit targets are more common, and this was already the case a couple of years ago.

Expected behavior 🤔

Since gitoxide is most often used on 64-bit targets, progress.rs should not state "we target 32bit systems only."

Secondarily, the entry for core.packedGitLimit should more clearly explain why it isn't applicable. However, changing the string to just say "we don't use a windowing mechanism" would be sufficient to fix the main issue.

Git behavior

Since the underlying context is documenting how and why gitoxide and Git differ with respect whether core.packedGitLimit is supported, the Git behavior, in documentation or otherwise, may not be directly relevant.

However, the Git documentation on core.packedGitLimit is linked above.

Steps to reproduce 🕹

Examine progress.rs as linked and quoted above.

Byron commented 1 month ago

Thanks for reporting, indeed this should say 64bit, not 32bit.

EliahKagan commented 1 month ago

Oh, interesting. Does this mean users on 32-bit systems, or who otherwise run a 32-bit build, should expect memory exhaustion to occur in large repositories with packed objects?

Byron commented 1 month ago

Yes, repositories can't go beyond a certain size on 32 bit systems due to virtual memory exhaustion.

EliahKagan commented 1 month ago

Assuming this is only a matter of the available address space for the process--since I think paging should take care of preventing excessive memory use even when memory-mapping large files--I think this may actually already be documented, but I am not sure.

I recall something to the effect that a 32-bit build of gitoxide does not handle huge individual files/blobs in a repository, except when LFS is used. But I am not sure if this is currently clarified to apply also to large packs even when individual objects are all smaller.

In any case, for progress.rs, I'll open a small PR shortly that fixes the wording to say "64-bit" and that may also make small clarifying changes elsewhere in that module.

Byron commented 1 month ago

I recall something to the effect that a 32-bit build of gitoxide does not handle huge individual files/blobs in a repository, except when LFS is used. But I am not sure if this is currently clarified to apply also to large packs even when individual objects are all smaller.

It only applies to large packs as these are memory mapped, but there are other places where mmaps are used that in theory have the same limits. This should be documented in plenty of places, but the spot this issue is about definitely needs adjustments :). Looking forward to the PR.

EliahKagan commented 1 month ago

I've opened #1531 for this.