Re: [openstack-dev] [all] Proper use of 'git review -R'
On Tue, Dec 30, 2014 at 11:24 AM, Jeremy Stanley wrote: > On 2014-12-30 12:31:35 -0500 (-0500), David Kranz wrote: > [...] >> 1. This is really a UI issue, and one that is experienced by many. >> What is desired is an option to look at different revisions of the >> patch that show only what the author actually changed, unless >> there was a conflict. > > I'm not sure it's entirely a UI issue. It runs deeper. There simply > isn't enough metadata in Git to separate intentional edits from > edits made to solve merge conflicts. Using merge commits instead of > rebases mostly solves this particular problem but at the expense of > introducing all sorts of new ones. A rebase-oriented workflow makes > it easier for merge conflicts to be resolved along the way, instead > of potentially nullifying valuable review effort at the very end > when it comes time to approve the change and it's no longer relevant > to the target branch. Jeremy is correct here. I've dreamed about how to enhance git to support this sort of thing more formally but it isn't an easy problem and wouldn't help us in the short term anyway. To overcome this, I hacked out a script [1] which rebases older patch sets to the same parent as the most current patch set to help me compare across rebases. I've found it very handy in certain situations. I can see how conflicts were handled as well as what other changes were made outside the scope of merge conflict resolution. I use it by downloading the latest patch set with "git review -d X" and then I compare to a previous patch set (NN) by supplying that patch set number on the command line. I once had dreams of adding this capability to gerrit but I found the gerrit development learning curve to be a bit steep for the time I had. > There is a potential work-around, though it currently involves some > manual effort (not sure whether it would be sane to automate as a > feature of git-review). When you notice your change conflicts and > will need a rebase, first reset and stash your change, then reset > --hard to the previous patchset already in Gerrit, then rebase that > and push it (solving the merge conflicts if any), then pop your > stashed edits (solving any subsequent merge conflicts) and finally > push that as yet another patchset. This separates the rebase from > your intentional modifications though at the cost of rather a lot of > extra work. > > Alternatively you could push your edits with git review -R and > _then_ follow up with another patchset rebasing on the target branch > and resolving the merge conflicts. Possibly slightly easier? I'm a strong proponent of splitting rebases (with merge conflict resolution) from other manual changes. This is a help to reviewers. If someone tells me that a patch set is a pure rebase to resolve conflicts then I can "review" it by repeating the rebase myself to see if I get the same answer. Both suggestions above are good ones. Which one you use is a matter of preference IMO. I personally prefer the latter (push with -R and then resolve conflicts) because it is easier on me. >> 2. Using -R is dangerous unless you really know what you are >> doing. The doc string makes it sound like an innocuous way to help >> reviewers. > > Not necessarily dangerous, but it does allow you to push changes > which are just going to flat fail all jobs because they can't be > merged to the target branch to get tested. I agree there is no danger. As I've stated in my other post, I have *always* used it for two years and have seen no danger. I have come to accept the failing jobs as a regular and welcome part of my work flow. If these failing jobs are taking a lot of resources then we need some redesign in infrastructure to fail them more quickly and cheaply so that resources can be spared from having to test patch sets which are in conflict. Carl [1] http://paste.openstack.org/show/155614/ ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
On Tue, Dec 30, 2014 at 9:37 AM, Jeremy Stanley 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
Re: [openstack-dev] [all] Proper use of 'git review -R'
On 12/30/2014 11:47 AM, Jeremy Stanley wrote: On 2014-12-30 10:32:22 -0600 (-0600), Dolph Mathews wrote: The default behavior, rebasing automatically, is the sane default to avoid having developers run into unexpected merge conflicts on new patch submissions. Please show me an example of this in the wild. I suspect a lot of reviewers are placing blame on this without due investigation. I think you msiread the intention of what Dolph posted, Jeremy. What he is saying is that "automatic rebasing ensures that a submitted patch would not get a false negative on a rebase problem." Or, to try to make it even clearer, the default behaviour forces the submitter to deal with rebase problems in their own sand box. But if git-review can check to see if a review already exists in gerrit *before* doing the local rebase, I'd be in favor of it skipping the rebase by default if the review already exists. Require developers to rebase existing patches manually. (This is my way of saying I can't think of a good answer to your question.) It already requires contributors to take manual action--it will not automatically rebase and then push that without additional steps. What I would like to see is this: 1. Rebase the last patch. (if possible) 2. Submit the new patch Now a reviewer can see the difference between what was actually submitted and the previous patch. If step 1 fails (it often does using the git review option for diff between two versions of the patch) just accept it and move on. While we're on the topic, it's also worth noting that --no-rebase becomes critically important when a patch in the review sequence has already been approved, because the entire series will be rebased, potentially pulling patches out of the gate, clearing the Workflow+1 bit, and resetting the gate (probably unnecessarily). A tweak to the default behavior would help avoid this scenario. The only thing -R is going to accomplish is people uploading changes which can never pass because they're merge-conflicting with the target branch. It makes it clearer what the diff is without complicating it with unrelated changes, which is what David wants to make happen. Ideally, any user that did a -R would immediately do a rebase as well, but that would complicate things for the reviewer. This is a common problem, and not a trivial one to solve. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
On 2014-12-30 12:31:35 -0500 (-0500), David Kranz wrote: [...] > 1. This is really a UI issue, and one that is experienced by many. > What is desired is an option to look at different revisions of the > patch that show only what the author actually changed, unless > there was a conflict. I'm not sure it's entirely a UI issue. It runs deeper. There simply isn't enough metadata in Git to separate intentional edits from edits made to solve merge conflicts. Using merge commits instead of rebases mostly solves this particular problem but at the expense of introducing all sorts of new ones. A rebase-oriented workflow makes it easier for merge conflicts to be resolved along the way, instead of potentially nullifying valuable review effort at the very end when it comes time to approve the change and it's no longer relevant to the target branch. There is a potential work-around, though it currently involves some manual effort (not sure whether it would be sane to automate as a feature of git-review). When you notice your change conflicts and will need a rebase, first reset and stash your change, then reset --hard to the previous patchset already in Gerrit, then rebase that and push it (solving the merge conflicts if any), then pop your stashed edits (solving any subsequent merge conflicts) and finally push that as yet another patchset. This separates the rebase from your intentional modifications though at the cost of rather a lot of extra work. Alternatively you could push your edits with git review -R and _then_ follow up with another patchset rebasing on the target branch and resolving the merge conflicts. Possibly slightly easier? > 2. Using -R is dangerous unless you really know what you are > doing. The doc string makes it sound like an innocuous way to help > reviewers. Not necessarily dangerous, but it does allow you to push changes which are just going to flat fail all jobs because they can't be merged to the target branch to get tested. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
On 12/30/2014 11:37 AM, Jeremy Stanley 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. From git-review doc: -R, --no-rebase Do not automatically perform a rebase before submitting the change to Gerrit. When submitting a change for review, you will usually want it to be based on the tip of upstream branch in order to avoid possible conflicts. When amending a change and rebasing the new patchset, the Gerrit web interface will show a difference between the two patchsets which contains all commits in between. This may confuse many reviewers that would expect to see a much simpler differ‐ ence. While not entirely incorrect, it could stand to be updated with slightly more clarification around the fact that git-review (since around 1.16 a few years ago) does not push an automatically rebased change for you unless you are using -F/--force-rebase. If you are finding changes which are gratuitously rebased, this is likely either from a contributor who does not use the recommended change update workflow, has modified their rebase settings or perhaps is running a very, very old git-review version. Thanks for the replies. The rebases I was referring to are not gratuitous, they just make it harder for the reviewer. I take a few things away from this. 1. This is really a UI issue, and one that is experienced by many. What is desired is an option to look at different revisions of the patch that show only what the author actually changed, unless there was a conflict. 2. Using -R is dangerous unless you really know what you are doing. The doc string makes it sound like an innocuous way to help reviewers. -David ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
On Tue, Dec 30, 2014 at 8:46 AM, David Kranz wrote: > Many times when I review a revision of an existing patch, I can't see just > the change from the previous version due to other rebases. I've gotten used to this. Typically when I review a new patch set I look for my comments and make sure they were addressed. Then I go back to compare with the base revision and look through the patch again. It's quicker this time since I remember what it was about. The git-review documentation mentions this issue and suggests using -R to > make life easier for reviewers when submitting new revisions. Can some one > explain when we should *not* use -R after doing 'git commit --amend'? A developer updating a patch is going to want to test the change using the latest master and not an old buggy version, so before you make your changes you rebase and -R isn't going to make a difference. You could be really considerate and re-rebase on the original parent. (Or you could be lazy and not test your changes locally with the latest master.) When you have a chain of commits rebasing gets more complicated since gerrit shows a dependent review as out of date if the parent review is changed in any way, and if there's a merge conflict far down the chain you have to rebase the whole chain. Rebasing the patch on master does make one thing easier -- if you want to download the patch and try it out and your newer environment (tox or devstack for example) doesn't work with the patch repo's old environment you'll need to rebase first to get it to work. Or is using -R just something that should be done but many folks don't know > about it? > > -David > > From git-review doc: > > -R, --no-rebase > Do not automatically perform a rebase before submitting the > change to Gerrit. > > When submitting a change for review, you will usually want it to > be based on the tip of upstream branch in order to avoid possible > conflicts. When amending a change and rebasing the new patchset, > the Gerrit web interface will show a difference between the two > patchsets which contains all commits in between. This may confuse > many reviewers that would expect to see a much simpler differ‐ > ence. > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
On 2014-12-30 10:32:22 -0600 (-0600), Dolph Mathews wrote: > The default behavior, rebasing automatically, is the sane default > to avoid having developers run into unexpected merge conflicts on > new patch submissions. Please show me an example of this in the wild. I suspect a lot of reviewers are placing blame on this without due investigation. > But if git-review can check to see if a review already exists in > gerrit *before* doing the local rebase, I'd be in favor of it > skipping the rebase by default if the review already exists. > Require developers to rebase existing patches manually. (This is > my way of saying I can't think of a good answer to your question.) It already requires contributors to take manual action--it will not automatically rebase and then push that without additional steps. > While we're on the topic, it's also worth noting that --no-rebase > becomes critically important when a patch in the review sequence > has already been approved, because the entire series will be > rebased, potentially pulling patches out of the gate, clearing the > Workflow+1 bit, and resetting the gate (probably unnecessarily). A > tweak to the default behavior would help avoid this scenario. The only thing -R is going to accomplish is people uploading changes which can never pass because they're merge-conflicting with the target branch. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
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. > From git-review doc: > > -R, --no-rebase > Do not automatically perform a rebase before submitting the > change to Gerrit. > > When submitting a change for review, you will usually want it to > be based on the tip of upstream branch in order to avoid possible > conflicts. When amending a change and rebasing the new patchset, > the Gerrit web interface will show a difference between the two > patchsets which contains all commits in between. This may confuse > many reviewers that would expect to see a much simpler differ‐ > ence. While not entirely incorrect, it could stand to be updated with slightly more clarification around the fact that git-review (since around 1.16 a few years ago) does not push an automatically rebased change for you unless you are using -F/--force-rebase. If you are finding changes which are gratuitously rebased, this is likely either from a contributor who does not use the recommended change update workflow, has modified their rebase settings or perhaps is running a very, very old git-review version. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [all] Proper use of 'git review -R'
The default behavior, rebasing automatically, is the sane default to avoid having developers run into unexpected merge conflicts on new patch submissions. But if git-review can check to see if a review already exists in gerrit *before* doing the local rebase, I'd be in favor of it skipping the rebase by default if the review already exists. Require developers to rebase existing patches manually. (This is my way of saying I can't think of a good answer to your question.) While we're on the topic, it's also worth noting that --no-rebase becomes critically important when a patch in the review sequence has already been approved, because the entire series will be rebased, potentially pulling patches out of the gate, clearing the Workflow+1 bit, and resetting the gate (probably unnecessarily). A tweak to the default behavior would help avoid this scenario. -Dolph On Tue, Dec 30, 2014 at 8:46 AM, David Kranz wrote: > Many times when I review a revision of an existing patch, I can't see just > the change from the previous version due to other rebases. The git-review > documentation mentions this issue and suggests using -R to make life easier > for reviewers when submitting new revisions. Can some one explain when we > should *not* use -R after doing 'git commit --amend'? Or is using -R just > something that should be done but many folks don't know about it? > > -David > > From git-review doc: > > -R, --no-rebase > Do not automatically perform a rebase before submitting the > change to Gerrit. > > When submitting a change for review, you will usually want it to > be based on the tip of upstream branch in order to avoid possible > conflicts. When amending a change and rebasing the new patchset, > the Gerrit web interface will show a difference between the two > patchsets which contains all commits in between. This may confuse > many reviewers that would expect to see a much simpler differ‐ > ence. > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
