Skip to content
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

fix: ignore .gitignore files that are directories #1386

Merged
merged 2 commits into from
May 28, 2024
Merged

fix: ignore .gitignore files that are directories #1386

merged 2 commits into from
May 28, 2024

Conversation

Byron
Copy link
Member

@Byron Byron commented May 28, 2024

This happens in real-life, and Git itself doesn't budge.

Related to gitbutlerapp/gitbutler#3876

Byron added 2 commits May 28, 2024 21:36
This happens in real-life, and Git itself doesn't budge.
@Byron Byron merged commit 31d2f0a into main May 28, 2024
19 checks passed
@Byron Byron deleted the dir-as-ignore branch May 28, 2024 20:02
@@ -59,7 +59,7 @@ pub fn create(original: &Path, link: &Path) -> io::Result<()> {
pub fn is_collision_error(err: &std::io::Error) -> bool {
// TODO: use ::IsDirectory as well when stabilized instead of raw_os_error(), and ::FileSystemLoop respectively
err.kind() == AlreadyExists
|| err.raw_os_error() == Some(21)
|| err.raw_os_error() == Some(if cfg!(windows) { 5 } else { 21 })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function here appears to be under #[cfg(not(windows))] making cfg!(windows) always false.
The #[cfg(windows)] below should probably be changed instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great catch, thank you!

I pushed the fix into this standing PR.

@EliahKagan
Copy link
Member

Should something be done so that new binary installations of gitoxide (i.e., pre-built gix and ein binaries) use the new dependencies?

When installed from source using cargo install gitoxide, or with cargo install --path . from a clone of the gitoxide repository, commands like gix status no longer fail due to the presence of a .gitignore directory. They behave as expected, no longer being tripped up by a the presence of such a directory:

% git ls-files
.gitignore/a
% gix status
 12:07:41 status done 1.0 files in 0.01s (183.0 files/s)

head -> index isn't implemented yet
% echo "$?"
0

But when installing with cargo quickinstall gitoxide or cargo binstall gitoxide, this still happens:

% git ls-files
.gitignore/a
% gix status
Error: Failed to update the excludes stack to see if a path is excluded
% echo "$?"
1

In both cases, this is with gitoxide 0.36.0.

It may be that nothing should be done. If the main affected subcommand is status, then the behavior of gix could perhaps wait until #1368 is done and merged, anyway. On the other hand, gix commands having two different behaviors even at the same version of gitoxide installed, due to non-intended differences in dependency versions, could be confusing. Then again, directories named .gitignore are uncommon. I'm not sure.

@Byron
Copy link
Member Author

Byron commented May 31, 2024

Thanks for making me aware of this.

I think the only way to assure such updates become available in cargo quickinstall is to create a dummy gitoxide patch release to incorporate them and I think for important (i.e. security related) fixes this should certainly be done one way or another.

For this one, I think it's OK to leave it as is and do nothing.

@NobodyXu
Copy link
Contributor

I think the only way to assure such updates become available in cargo quickinstall is to create a dummy gitoxide patch release to incorporate them

I could delete the cargo-quickinstall release for gitoxide, that should trigger a rebuild later.

@Byron
Copy link
Member Author

Byron commented May 31, 2024

If you would like to practice this workflow on that occasion, I'd take you upon that offer. Maybe one day that will come in handy for emergency fixes of sorts.
Thanks again!

@NobodyXu
Copy link
Contributor

Removed gitoxide from cargo-quickinstall, waiting for it to rebuild.

@EliahKagan
Copy link
Member

EliahKagan commented Jul 20, 2024

This happens in real-life, and Git itself doesn't budge.

While git permits this on all systems, on Windows it gives a warning that creates the appearance that it does not. I've opened git-for-windows/git#5068 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants