Gitmanager refactor #1216
Gitmanager refactor #1216
Conversation
|
This also eliminates the notorious "HEAD isn't at the tip of origin's master branch" error. |
|
I'm here but I'm waiting for the revamp which avoids creating a branch for privileged users. https://chat.stackexchange.com/transcript/message/41039077#41039077 |
ws.py
Outdated
| import os.path | ||
|
|
||
| if not os.path.islink(".git/hooks/pre-commit"): | ||
| os.symlink("hooks/pre-commit", ".git/hooks/pre-commit") |
There was a problem hiding this comment.
What about copying hooks/* into .git/hooks/ on each launch? This keeps the scripts up-to-date without the issues surrounding symlinks.
There was a problem hiding this comment.
Personally I thought that would be more inconvenient than a symlink. At any rate, I found a better way: you can set a custom hook directory from git config.
| git.checkout("-b", branch) | ||
|
|
||
| # Clear HEAD just in case | ||
| git.reset("HEAD") |
There was a problem hiding this comment.
This should be kept somewhere. We need to make sure we're actually on HEAD and not in a detached or reverted state when we start working on data in the code at all. Which is incredibly important.
There was a problem hiding this comment.
In your test case in CHQ room, you detailed why this is important - you clobber remote commits that may be just as important. Therein lies the core problem and why we left this here.
There was a problem hiding this comment.
| @@ -148,13 +130,13 @@ def add_to_blacklist(cls, **kwargs): | |||
| "-m", u"Auto {0} of {1} by {2} --autopull".format(op, item, username)) | |||
|
|
|||
| if code_permissions: | |||
| git.checkout("master") | |||
| git.merge(branch) | |||
There was a problem hiding this comment.
So we're letting GH handle all of the commits then? We had this here because if you have code privs you should already push this but also locally merge it. As you refactor it, this will prevent in-place activation of changed things if one has code perms.
There was a problem hiding this comment.
This also was built to push to master directly, hence why we checked out master to begin with. your changes will fubar this process.
There was a problem hiding this comment.
This is a valid point, however I'm inclined to prefer that we wait for CI.
|
Pulling this out of an embedded comment, discussion about this in CHQ: https://chat.stackexchange.com/rooms/11540/conversation/pr-1216 |
There was a problem hiding this comment.
I too participated in the Charcoal discussion here:
https://chat.stackexchange.com/rooms/11540/conversation/review-discussion-for-pr1216
In summary, the rough consensus is that we need to avoid losing commits to the local master, or be thoroughly convinced that there are no scenarios where such commits can contain useful work.
I like the general thinking here, and would love to see a more versatile and robust Git manager. But I think our first task needs to be refactoring the current code to something we can create a test suite for, and only then start experimenting with different concurrency models. I am quite sure we will uncover additional failure scenarios in the current code which we are not well aware of.
|
As we're currently discussing in chat, I think there might be a bit of a misunderstanding here. Nothing about this will clobber remote commits -- the only window of opportunity for a remote commit to cause a conflict is between the fetch and the push which is a few seconds at max, and if it did occur the push would simply fail (Git only tries to fast forward commits when pushing, it'll give up if it needs to merge them.) As for local commits, they're all still there -- they just won't be pushed with the blacklist (by design). Presumably they were created by a human, so a human should resolve and push them. |
|
I should add that the |
|
I've set up another repository with a clean Git tree with my modifications, and confirmed that blacklisting (at least calling |
|
Pull requests also work, and no local deploy or master changes were harmed: quartata/smokey-git-test#1 |
This does two things to help improve Smokey's Git workflow:
Note that at no point do we commit directly to the local master or even check it out. This also eliminates any merging, which would run into issues if the changes couldn't be fast-forwarded.