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

add support for the git option "index.skipHash" #1488

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rggjan
Copy link

@rggjan rggjan commented Feb 2, 2025

Here is the comparable implementation in another git library for adding support for the same feature: https://github.com/libgit2/libgit2/pull/6738/files

@rggjan rggjan requested a review from jelmer as a code owner February 2, 2025 17:48
@rggjan rggjan mentioned this pull request Feb 2, 2025
Copy link
Owner

@jelmer jelmer left a comment

Choose a reason for hiding this comment

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

Shouldn't this be checking the configuration flag?

@rggjan
Copy link
Author

rggjan commented Feb 4, 2025

Shouldn't this be checking the configuration flag?

In libgit, they also don’t seem to check the configuration flig: https://github.com/libgit2/libgit2/pull/6738/files

It seems to me that the checksum is all 0 in the index if the client used to write that index had skipHash enabled, which is probaby independent of the flags set at the time when it is read by dulwich?

# If git option index.skipHash is set the index will be empty
if stored != self.sha1.digest() and (
not allow_empty
or sha_to_hex(stored) != b"0000000000000000000000000000000000000000"
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably use the ZERO_SHA constant

@jelmer
Copy link
Owner

jelmer commented Feb 4, 2025

Shouldn't this be checking the configuration flag?

In libgit, they also don’t seem to check the configuration flig: https://github.com/libgit2/libgit2/pull/6738/files

It seems to me that the checksum is all 0 in the index if the client used to write that index had skipHash enabled, which is probaby independent of the flags set at the time when it is read by dulwich?

They do set skipHash in the tests though: https://github.com/libgit2/libgit2/blob/913d4ea251366f006a5ced6ed1804acf973a6ce6/tests/resources/status_skiphash/.gitted/config#L8

@jelmer
Copy link
Owner

jelmer commented Feb 4, 2025

Actually, I guess this is okay - but can you perhaps add support for skipHash on the creation side? We've been trying to remove the testdata files, so I'd rather not add any more.

@jelmer jelmer enabled auto-merge February 4, 2025 16:24
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.

2 participants