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

do checkout before changing HEAD #20916

Merged
merged 4 commits into from
Mar 8, 2017
Merged

do checkout before changing HEAD #20916

merged 4 commits into from
Mar 8, 2017

Conversation

simonbyrne
Copy link
Contributor

@simonbyrne simonbyrne commented Mar 6, 2017

According to the libgit2 docs, we should call git_checkout_* before changing the HEAD ref. This is generally not a big problem as Pkg does a force checkout (which is probably not a good idea either, but that's for another day), except in a few edge cases.

Fixes both #17610 and #18420. Unfortunately, I have no idea how to test this.

@simonbyrne simonbyrne added the libgit2 The libgit2 library or the LibGit2 stdlib module label Mar 6, 2017
@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

maybe we should try to create a test repo that hits one of those corner cases with permissions or file types changing?

@simonbyrne
Copy link
Contributor Author

I guess, but is there a way to do it so that it works across all platforms?

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

I don't think these issues were a problem on all platforms though. I'll have to test if they occur at all on Windows.

@simonbyrne
Copy link
Contributor Author

Given the changes to LibGit2, a backport will need to be a bit different. Do you want me to put together another PR?

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

let's see if/when 0.5.2 ends up being necessary before spending time on it, if non-trivial

@simonbyrne
Copy link
Contributor Author

added a test.

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

yay, thank you!

@simonbyrne simonbyrne mentioned this pull request Mar 6, 2017
42 tasks
@tkelman tkelman merged commit 78b96d0 into master Mar 8, 2017
@tkelman tkelman deleted the sb/libgit2/mads branch March 8, 2017 03:52
@tkelman
Copy link
Contributor

tkelman commented Mar 27, 2017

What would the backport version of this look like?

simonbyrne added a commit that referenced this pull request Mar 27, 2017
* do checkout before changing HEAD

* add test for change to file permissions

* add test for symlinks

* add issue numbers to comments
@simonbyrne
Copy link
Contributor Author

I guess just changing the order of checkout_tree and GitReference. Try the sb/libgit2/mads-backport branch (I haven't tested it yet)

@tkelman
Copy link
Contributor

tkelman commented Mar 27, 2017

Very nice, thank you, that does appear to fix the issue on 0.5. Will add the cherry-pick -x cross-reference and run that through PkgEval when I prepare 0.5.2.

tkelman pushed a commit that referenced this pull request May 2, 2017
* do checkout before changing HEAD
541d94b

* add test for change to file permissions
42493ae

* add test for symlinks
a3fab6b

* add issue numbers to comments
1b7d47a
tkelman pushed a commit that referenced this pull request May 3, 2017
* do checkout before changing HEAD
541d94b

* add test for change to file permissions
42493ae

* add test for symlinks
a3fab6b

* add issue numbers to comments
1b7d47a

change close(repo) to finalize(repo) for release-0.5
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.

2 participants