Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Use flock() for file locking#513

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Byron merged 11 commits intogitpython-developers:masterfromexpobrain:master
Oct 9, 2016

Conversation

expobrain
Copy link

@expobrainexpobrain commentedSep 14, 2016
edited by ankostis
Loading

The problem with the original code is that the lock was based on the presence of the lockfile, that is if the lockfile exists the _obtain_lock() fails otherwise it succeed on acquiring the lock.

However this was problematic because between the check if the lockfile exists and the creation of the file another process can create the file as well resulting in a double lock, and we don't want to lock the same file twice :-)

@Byron
Copy link
Member

Thanks@expobrain for this valuable contribution, and@Alexxz for the initial review.

Having glanced over the code, I'd believe it has a chance to work given that all unit-tests still succeed. However, I am not quite sure whether I would be willing to downright kill windows support, asfcntl doesn't seem to be supported there. It's a breaking change, asking for a major version increment at least. The alternative could be to have a branching code-paths, which again would be difficult to test on windows as there is no CI setup for it.

Being very honest to myself, then admittedly I don't have access to windows and will not make such an attempt. Maybe there is someone working on windows who would be willing to take over maintaining support for that platform.

I also believe that a major version increment would be a doable precondition for a merge.
@gitpython-developers/contributors (and everyone inclined) please be welcome to chime in, I would not want to make this decision alone. Until there is a consensus, I would refrain from merging this one.

@expobrain
Copy link
Author

Hi Byron, I'm more than happy to add the Windows support of this PR. Do you have any guidelines on how to develop and test GitPython on Windows?

@Byron
Copy link
Member

@expobrain Thanks a lot in advance ! Any help is appreciated and welcome. There are not many requirements when getting started contributing, and the few things there are have been added tothe contribution guide.

@ankostis Is working on integrating GitPython with AppVeyor as CI for windows in PR#519, which I believe would be very beneficial for you to have.

Alternatively, you just get started and focus on the tests that verify the locking specifically. Once they run on your machine locally, you could just push them into your PR to have travis verify nothing broke on the linux side.

I hope you find this answer helpful, and that you can get started painlessly.

@ankostis
Copy link
Contributor

#519 is a gargatuan PR that I've been working for almost 2 weeks.
It contains fixes for Appveyor to pass all green, but even them may not be enough for Windows. Extra TCs are needed; for instance, there are tests that fail on my machine and pass in Apveyor. Wozniak demands only the best from us programmers :-)

@ankostis
Copy link
Contributor

@expobrain can you rebase on master now that#519 is merged?

@ankostisankostisforce-pushed themaster branch 2 times, most recently from391a767 to8a2f7dcCompareOctober 4, 2016 00:12
@expobrain
Copy link
Author

Yes, doing it now

@Byron
Copy link
Member

Thanks a lot for your contribution, and to everyone else who has contributed to this PR !

@ByronByron merged commitac9ac55 intogitpython-developers:masterOct 9, 2016
@ankostis
Copy link
Contributor

ankostis commentedOct 11, 2016
edited
Loading

@Byron Appveyor fails on this :-(https://ci.appveyor.com/project/ankostis/gitpython/build/1.0.225

Have you logged-into Appveyor and add this project?

@ankostis
Copy link
Contributor

ankostis commentedOct 11, 2016
edited
Loading

@Byron This PR had caused me manyWindows problems downstream - please revert.

@expobrain If the old locking-code is problematic, we may look also into this alternative library:https://github.com/pakal/rsfile/blob/master/doc/locking_semantic.rst

But I really miss an explanation what was the problem with the old code (add it in the OP).

@ankostis
Copy link
Contributor

Actually this PR has been reverted.

@expobrain if you feel like you can address some of the issue we can revisit this one; but please first explain what was the problem with the old code.
For start, I can give my own input on this:
it does not provide a resource contextlib-management API (with ...: construct). So maybe a better solution would be to tackle both issues, and consider alsothe library mentioned in the comments of the site wherer you got the code (thanks for keepingreferences to your job).

@expobrain
Copy link
Author

@ankostis the problem with theoriginal code is that the lock was based on the presence of the lockfile, that is if the lockfile exists the_obtain_lock() fails otherwise it succeed on acquiring the lock.

However this was problematic because between the check if the lockfile exists and the creation of the file another process can create the file as well resulting in a double lock, and we don't want to lock the same file twice :-)

Thats all, I hope it makes sense.

Thank you also to point us to the library, I'll have a look because looks more complete than my PR and also supports Windows out of the box.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@expobrain@Byron@ankostis@Alexxz

[8]ページ先頭

©2009-2025 Movatter.jp