4 ms·

> say the doc string has a typo and you already merged in, are you suggesting you rather see a separate commit rather than rebase it You probably mean amending

> say the doc string has a typo and you already merged in, are you suggesting you rather see a separate commit rather than rebase it You probably mean amending an existing commit, not rebasing. Gerrit encourages repeatedly amending a commit to incorporate review feedback, thereby erasing history from git. And why NOT have the typo fix as a separate commit? Are you paying by the SHA1 hash? The thing is, you aren't really keeping a clean history, you're just redirecting it to another, less flexible abstraction layer. The complete history is still in gerrit for anyone to look at, but now it can't participate in git blame, git bisect, etc. > For real project, before I merge in, a code review is necessary But this is just begging the question. Why is it necessary to do a code review before a merge, instead of after?

yeukhon 12y ago
> You probably mean amending an existing commit, not rebasing. Hmm yes correct. I have never used Gerrit so I don't know what that whole fuzz is about. I am more into what he meant by rebase is bad. > But this is just begging the question. Why is it necessary to do a code review before a merge, instead of after? By merging I am talking about merging with the master repo, or upstream in Git's world. You work on your local repo, preferably a new branch so you can work on multiple stories (I use branch as patch). Before I want to merge into the upstream for real professional project, I want to do a proper code review. If I work with people using Mercurial, I prefer people use patch queue. I don't like history to be ugly. I want each history has its own meaning. Why? Because when I blame I want to narrow down quickly (yes there are ways to do that really fast, but let's stick to just browsing on Github). If you edit the same line twenty lines just to fix a small typo, wow, that's not cool. I've dealt with projects and some people like this: fix typo fix typo2 fix as suggested in comment 2 fix as suggested in comment 5 This is particularly painful in Mercurial if you don't use patch. Rebase in mercurial is pretty tough for me, at least my experience. So I apply the same logic to git: branch, rebase and submit PR, bug found, back to branch, fix, rebase, repeat. In the end what I care about is the commit links to the issue # and a good commit message. Would you rather read the commits above or "Fixed #1913 XXXXXXXXXXX"? I try to make sure every commit follows something like "Fixed #" in my professional repo. Sometimes I fail to do that, but I try my best. It's my workflow and I think it's reasonable. --EDIT-- I just want to make sure we are clear about professional vs personal. In a professional OSS project, there are many users and so rebasing on a master repo is not acceptable because someone might already working on the older version. The only time I will ever rebase the master commit is if I need to remove "sensitive data" leak (yes, at that point I am screwed because someone might have checked out the original commit). In a professional setting, a typo found in master commit should be filed as a new bug and fixed as a new commit. In a personal project it's up to me to decide. I can be lazy like committing hundreds of "fix this fix that." But I usually don't.
dkuntz2 12y ago
The main argument against not having a separate commit is that with large projects the history becomes useless when every commit is type fixing. In the case of fixing typos or making tiny changes that don't really deserve their own commit, the person making the tiny changes should also be the person who made the last big changes too. --- > Why is it necessary to do a code review before a merge, instead of after? Because most of the time you want master to be working, and working as well as possible. In a team environment, you shouldn't all just be throwing everything on master, that leads to unnecessarily crazy history. You review it before to say "alright, this is good, we all like this, this could be placed in production now if we wanted".

yeukhon 12y ago

> You probably mean amending an existing commit, not rebasing. Hmm yes correct. I have never used Gerrit so I don't know what that whole fuzz is about. I am more into what he meant by rebase is bad. > But this is just begging the question. Why is it necessary to do a code review before a merge, instead of after? By merging I am talking about merging with the master repo, or upstream in Git's world. You work on your local repo, preferably a new branch so you can work on multiple stories (I use branch as patch). Before I want to merge into the upstream for real professional project, I want to do a proper code review. If I work with people using Mercurial, I prefer people use patch queue. I don't like history to be ugly. I want each history has its own meaning. Why? Because when I blame I want to narrow down quickly (yes there are ways to do that really fast, but let's stick to just browsing on Github). If you edit the same line twenty lines just to fix a small typo, wow, that's not cool. I've dealt with projects and some people like this: fix typo fix typo2 fix as suggested in comment 2 fix as suggested in comment 5 This is particularly painful in Mercurial if you don't use patch. Rebase in mercurial is pretty tough for me, at least my experience. So I apply the same logic to git: branch, rebase and submit PR, bug found, back to branch, fix, rebase, repeat. In the end what I care about is the commit links to the issue # and a good commit message. Would you rather read the commits above or "Fixed #1913 XXXXXXXXXXX"? I try to make sure every commit follows something like "Fixed #" in my professional repo. Sometimes I fail to do that, but I try my best. It's my workflow and I think it's reasonable. --EDIT-- I just want to make sure we are clear about professional vs personal. In a professional OSS project, there are many users and so rebasing on a master repo is not acceptable because someone might already working on the older version. The only time I will ever rebase the master commit is if I need to remove "sensitive data" leak (yes, at that point I am screwed because someone might have checked out the original commit). In a professional setting, a typo found in master commit should be filed as a new bug and fixed as a new commit. In a personal project it's up to me to decide. I can be lazy like committing hundreds of "fix this fix that." But I usually don't.

dkuntz2 12y ago

The main argument against not having a separate commit is that with large projects the history becomes useless when every commit is type fixing. In the case of fixing typos or making tiny changes that don't really deserve their own commit, the person making the tiny changes should also be the person who made the last big changes too. --- > Why is it necessary to do a code review before a merge, instead of after? Because most of the time you want master to be working, and working as well as possible. In a team environment, you shouldn't all just be throwing everything on master, that leads to unnecessarily crazy history. You review it before to say "alright, this is good, we all like this, this could be placed in production now if we wanted".