Re: [openstack-dev] [all] Proper use of 'git review -R'

2015-01-05 Thread Carl Baldwin
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


Re: [openstack-dev] [all] Proper use of 'git review -R'

2015-01-05 Thread Carl Baldwin
On Tue, Dec 30, 2014 at 11:24 AM, Jeremy Stanley fu...@yuggoth.org 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'

2015-01-05 Thread Adam Young

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'

2014-12-30 Thread Dolph Mathews
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 dkr...@redhat.com 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


Re: [openstack-dev] [all] Proper use of 'git review -R'

2014-12-30 Thread Jeremy Stanley
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'

2014-12-30 Thread Jeremy Stanley
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'

2014-12-30 Thread Brant Knudson
On Tue, Dec 30, 2014 at 8:46 AM, David Kranz dkr...@redhat.com 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'

2014-12-30 Thread David Kranz

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'

2014-12-30 Thread Jeremy Stanley
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