search

haystack / listit

The listit lightweight notetaking client
http://listit.csail.mit.edu/
MIT License
9 stars 6 forks source link

Fix issue #158 #215

Closed xiangcy closed 10 years ago

karger commented 10 years ago

If these three commits are addressing separate issues, it's generally better practice to submit three separate pull requests, so that each can be pulled or rejected separately.

If these three commits are all addressing the same issue, it may be useful (but not always) to "squash" these commits using git rebase (see e.g. http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html ) On 10/21/2013 7:37 PM, xiangcy wrote:

    You can merge this Pull Request by running

git pull https://github.com/xiangcy/listit loginpopup

Or view, comment on, or merge it at:

https://github.com/haystack/listit/pull/215

    Commit Summary
Stebalien commented 10 years ago

They are actually addressing 4 separate issues. 213b09aebfc6fdf545e27948a0311dcb437cbeff should be broken up into two commits. However, while nice, separate pull requests aren't really necessary; the request simply won't be merged until everything is fixed.

xiangcy commented 10 years ago

Delete all the platform-specific code, make the login popup listening to the registered state of the server, and rebase small commits into one big commit.

Stebalien commented 10 years ago

I see 7 commits...

xiangcy commented 10 years ago

That is because I already pushed some commits before and could not delete them remotely. Also when I rebase, I have to merge with the remote git repo. All the changes are in the commit https://github.com/xiangcy/listit/commit/d9735d19877eedf4e11089b4fbf4fd15c7413abb

Stebalien commented 10 years ago

Logical Commits

Keep it to one commit per meaningful step. In this case, you need two commits, one for each issue. Preferably, these commits would be in two separate pull requests in this case that's up to you.

Basically, if you have:

  1. Fixes bug in 2
  2. Fixes x
  3. Fixes y

You should have:

  1. Fixes x (just include the bug fix here)
  2. Fixes y

However, you shouldn't have:

  1. Fixes y and y

Rebasing

I have to merge with the remote git repo.

You don't. When finalizing a pull request, do the following:

  1. Rebase off of haystack/master (the upstream master branch).
  2. TEST. Commits made to upstream/master might break your changes.
  3. Rework your commits into a meaningful steps using git rebase -i (git rebase -i haystack/master should let you interactivity rebase all commits made on-top-of upstream). 4 Force push: git push -f. This will rewrite the remote history.

You don't really need to (and probably shouldn't) do this when simply revising a pull request (due to feedback) because it destroys history. However, when the pull request is ready to merge, I'll ask you to rebase and rework your commits so that the master history is clean.

xiangcy commented 10 years ago

Add a class "not-logged-in" for the main page, show the login warning when not logged in.

Stebalien commented 10 years ago

Nice! I'll merge it after you address my nitpick and rebase into two commits (one for each bug).

xiangcy commented 10 years ago

Done. Fix the last thing and rebase into one commit only for bug 158.

Stebalien commented 10 years ago

There are still two changes here: the platform specific help changes and the persistence warning. These should be in two separate commits.

xiangcy commented 10 years ago

Decouple into two separate commits. Wait for merging to the main branch!