Skip to content

use github fast path to check for changes before doing the git pull #47

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

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

syphar
Copy link
Contributor

@syphar syphar commented Oct 23, 2024

Coming from this zulip thread, looking at:

it seems to be recommended by github to use the fast path to check if there are changes, before actually doing the git pull.

This is for just just a (working) draft PR that needs some work around errors & tests, but I wanted to check with you if the approach is fine before I invest more work.

With this change I would probably change docs.rs to check the index more often, so we see requests more often.

@syphar
Copy link
Contributor Author

syphar commented Oct 23, 2024

@Byron what do you think?

@Byron
Copy link
Owner

Byron commented Oct 23, 2024

That sounds like a very good idea, please go for it!

There should also be some tests that clone from GitHub already on CI, so I'd expect the code to run already. Thus I'd think not much has to be done except for maybe assuring that it doesn't break non-GitHub.

For that, maybe have some unit tests around the 'is this a GitHub URL' to be sure it will work for other URLs as well, that should suffice.

@syphar syphar force-pushed the github-fast-path branch 2 times, most recently from 2e202d9 to 621bd78 Compare October 25, 2024 03:51
@syphar syphar marked this pull request as ready for review October 25, 2024 03:51
@syphar
Copy link
Contributor Author

syphar commented Oct 25, 2024

r? @Byron

@syphar
Copy link
Contributor Author

syphar commented Oct 25, 2024

btw, I removed the feature-flag again, thinking that we should actually never provide the option not to use the fast-path for github.

Of course we might have unnecessary dependencies for non-github indexes, though reqwest is already in the tree from gix-transport

@syphar
Copy link
Contributor Author

syphar commented Oct 25, 2024

Of course I can add it back and make it optional

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks a lot for contributing!

@Byron Byron merged commit 1ada2f1 into Byron:main Oct 25, 2024
1 check passed
@Byron
Copy link
Owner

Byron commented Oct 25, 2024

And here is a new release, with gix upgrade: https://github.com/Byron/crates-index-diff-rs/releases/tag/v26.0.0

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