On Tue, Dec 30, 2014 at 9:37 AM, Jeremy Stanley <fu...@yuggoth.org> wrote: > On 2014-12-30 09:46:35 -0500 (-0500), David Kranz wrote: > [...] >> Can some one explain when we should *not* use -R after doing 'git >> commit --amend'? > [...] > > In the standard workflow this should never be necessary. The default > behavior in git-review is to attempt a rebase and then undo it > before submitting. If the rebase shows merge conflicts, the push > will be averted and the user instructed to deal with those > conflicts. Using -R will skip this check and allow you to push > changes which can't merge due to conflicts.
tl;dr: I suggest an enhancement to git review which will help us avoid unintentionally uploading new patch sets when a change depends on another change. I've been thinking about this a bit since I had a discussion in the infra room last month. I have been using --no-rebase every time I run git review and I've been telling others to do the same. I even proposed setting defaultrebase to 0 for the neutron project [1]. At that time, I learned that this is expected to be the default for current versions of git review. I had a few experiences during the development of the DVR feature this past summer that leave me believing that there is still a problem. I saw a few cases where multiple authors were working on dependent patches and one author's rebase of an older dependency clobbered newer changes. This required me to step in and manually find and restore the clobbered changes. Things got better when I asked all of the authors to always use --no-rebase and we manually managed necessary rebases due to merge conflicts independently of other changes to the patch sets. I haven't had time to dig up all of the details about what happened. I will try to find some time to do that soon. However, I have an idea of where the problem is... The problem happens when a chain of dependencies is rebased together to master. This creates new versions of dependencies as well as the top patch. The "new" version of the dependency might actually be a rebased version of an older patch set. When this "new" version is uploaded, it clobbers changes to the dependency. I think this is generally the wrong thing to do; especially when a patch set chain has multiple authors. This is not the way gerrit rebases when you use the rebase button in the UI. Gerrit will rebase a patch set to the latest patch set of the change on which it depends. It there is no dependency, then it will rebase to master. I'm not sure if this is git review's fault or not. I know in older versions of git review it was at fault. More recent incidents could have been due to manually initiated rebases which were done incorrectly. However, I had the impression that git review would do rebases in this way and our problems on DVR seemed to stop when I trained the team to use --no-rebase. *** I can suggest an enhancement to git review which will help out in this situation. The change is around how git review warns about uploading multiple patch sets. It doesn't seem to be smart enough to tell when it will actually upload a new version of a dependency. That is, it warns even when the commit id of a dependency matches one that is already in gerrit as if it were going to create a new patch set. It is impossible to tell -- without manually checking out of band -- if it is *really* going to create a new patch set. I doubt many people (besides me) actually bother to go to gerrit to compare commit ids to see what will really happen. Git review should check the commit ids in gerrit. It should not stop to warn about commit ids that already exist in gerrit. Then, it should warn a bit *louder* about commit ids which are not in gerrit because many people have become desensitized to the current warning. Another habit that I have developed is to always download the latest version of a patch set, work on it fairly quickly, and then upload it again. I don't keep a lot of WIP locally for extended periods of time. I never know when someone is going to depend on a patch of mine and rebase it -- whether intentionally or not -- and upload the rebased version. I've dreamed about adding features to git/gerrit to manage patch set iterations within a change, and dependent changes more formally so that these problems can be more easily detected and handled by the tools. I think if git rebase could leave some sort of soft trail it might help but I haven't thought through this completely. I can see problems with how to handle this in a distributed way. Carl [1] https://review.openstack.org/#/c/140863/ _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
