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

Fix some basic lint issues in vitess/py, add effective_caller_id to vtgate_cursor interfaces. #1026

Merged
merged 13 commits into from
Aug 23, 2015

Conversation

dumbunny
Copy link
Contributor

@michael-berlin Lint-compliance changes, from dumbunny/vitess branch lint0 to youtube/vitess master.

@dumbunny
Copy link
Contributor Author

@michael-berlin I don't understand "This branch has conflicts that must be resolved". What are these conflicts?

@yaoshengzhe
Copy link
Contributor

It means your change has conflict with current master branch. Do a "git rebase master" to resolve conflict locally. It is pretty much like you and someone else touches same file and the other guy has merged change into master first, so you have to accommodate the change in your branch...

@dumbunny
Copy link
Contributor Author

I haven't figured it out. I might abandon this and make another branch and pull request.

@michael-berlin
Copy link
Contributor

Noo, don't do that :-)

I recommend that you merge the master into your feature branch at this point. Therefore, do as follows:

# If you haven't committed all your pending changes yet, stash them away.
# We do this because you must not have any pending changes before you switch branches.
# (Stash is an extra place in Git where you can temporarily stash away changes. We'll get the changes back later from there.)
git stash save

# Switch to the master branch.
git checkout master
# Get the latest changes.
git pull master

# Switch back to your feature branch.
git checkout lint0
# Merge master into it.
git merge master

# At this point, it's likely that conflicts have occurred. Check git status. It will tell you.
git status
# If there are conflicts, you can edit the files manually to resolve the conflicts.
# In order to tell Git that you have resolved the conflicts, follow its instructions in "git status".
# It probably asks you to "git add" the changed files and then "git commit" them. This way Git knows that you resolved the conflicts.
# Alternatively, you can run git mergetool to resolve the conflicts. I've configured my Git setup to use p4merge. That brings up a super nice UI where it's very easy to fix merge conflicts.
git mergetool

# At this point, you have merged the latest changes from master into your feature branch.
# What's left is to get your pending changes back from your stash:
git stash pop
# Here conflicts may occur again. If that's the case, you have to resolve them manually e.g. with "git mergetool" or by editing the files manually.

Let me know how that goes. You don't need to use the stash when you commit all pending changes before switching branches.

Regarding rebase vs. merge: This site seems to have good explanations for it. I recommend to read it: https://www.atlassian.com/git/tutorials/merging-vs-rebasing/conceptual-overview

For your workflow, I would say the following:

  • if your pull request hasn't received any comments yet, rebase your feature branch and force push new changes. In that case, previous commits get overridden in the pull request.
  • If your pull request has already received comments, I recommend to merge the master branch instead. This way, comments on existing commits won't be hidden. Also, the reviewer better sees which commits they already reviewed and which are new.

"""Base class for database classes and decorator for db method.

The base class DBObjectBase is the base class for all other database
base classes. It has methods for common database operations like
Copy link
Contributor

Choose a reason for hiding this comment

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

Text has double spaces after the period.

Please change them to a single space.

@dumbunny
Copy link
Contributor Author

$ git remote -v
origin git@github.com:youtube/vitess.git (fetch)
origin git@github.com:dumbunny/vitess.git (push)
upstream git@github.com:youtube/vitess.git (fetch)
upstream git@github.com:youtube/vitess.git (push)

Notice that I changed my origin to fetch from youtube/vitess.git and push to dumbunny/vitess.git. This seems to have unstuck me, but it is different from @michael-berlin's setup.

@michael-berlin
Copy link
Contributor

@dumbunny Yes, please change your remote setup such that origin fetch points to your "dumbunny" fork again.

You were both right that the changes for master were not pulled from "upstream", but from "origin".

Jeff's instructions can be simplified by letting master track the official repo and not the fork:

$ git branch master --set-upstream-to upstream/master

You can see the correct tracking in branch -vv

$ git branch -vv | grep master
* master                                  a1a9b57 [upstream/master] Merge pull request #1028 from michael-berlin/netutil_fixes

From now on, you can always omit "upstream" when pulling the master:

$ git checkout master
$ git pull

@michael-berlin
Copy link
Contributor

With the instructions in my last comment, you can omit "upstream" from a "git pull" while in "master".

With the instructions below, we also achieve that a "git push" no longer requires to specify a remote target.

$ git config remote.pushDefault origin
$ git config push.default current

The first command ensures that "git push" will always use the "origin" remote when nothing else is specified. This is great for two reasons:

  1. it's a safe guard against accidental "master" pushes. If you would accidentally push the "master" branch now, it would push to "origin" now and not to "upstream" anymore. (And that despite the fact that we just changed "master" to track "upstream".)
  2. When you push a feature branch now, it will automatically set up the tracking.

For example:

# You are on your master branch and pull the latest changes.
$ git checkout master
$ git pull
# You start working on a feature branch.
$ git checkout -b feature1
$ git commit -a
# Now you're ready to push the changes for review.
$ git push

At this point, "feature1" will be pushed to your fork ("origin" remote target) and not the "upstream" remote.

When you need to make changes, you would just run git push again.

For completeness: The second command git config push.default current is necessary to change Git's behavior how it associates local to remote branches. "current" means that the local and remote branch name will be the same. We have to set this explicitly because the default mode "simple" is more advanced and does not work with the "pushDefault" setting from the first command.

@dumbunny dumbunny changed the title Fix some basic lint issues in vitess/py. Fix some basic lint issues in vitess/py, add effective_caller_id to vtgate_cursor interfaces. Aug 23, 2015
@dumbunny
Copy link
Contributor Author

Merging.

dumbunny added a commit that referenced this pull request Aug 23, 2015
Fix some basic lint issues in vitess/py, add effective_caller_id to vtgate_cursor interfaces.
@dumbunny dumbunny merged commit 80dec80 into vitessio:master Aug 23, 2015
@dumbunny
Copy link
Contributor Author

(all edits addressed).

notfelineit pushed a commit to planetscale/vitess that referenced this pull request Sep 21, 2022
…o#1026)

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
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.

4 participants