Revert change feature creating changes that can't be submitted
Magnus Bäck
http://r.android.com/23019), and it's quite nice. However, I was
surprised to find that a commit created by this feature has the
commit being reverted as its parent. If you're reverting the most
recent commit on a branch (which probably often is the case) this
doesn't matter, but if the commit is older you may end up with
a) a version graph that looks unnecessariyl crappy and b) a change
that might not even merge unless it's rebased first.
Looking at the code, the observed behavior isn't surprising
(ChangeUtil.java, line ~293):
revertCommit.setTreeId(parentToCommitToRevert.getTree());
It's actually quite clever to implement this by reusing the parent
commit's tree, but I don't like the consequences -- if Gerrit creates
a commit for me I expect it to be submittable or that Gerrit at least
tells me that I'll have to rebase it myself first.
Of course, there's a race condition here. The millisecond after I
have Gerrit create the revert commit for me someone else might submit
a change that would cause a conflict, but Gerrit can at least make
its best for me.
If JGit has rebase machinery built in I'd like ChangeUtil.revert()
to rebase the created commit to the tip of the branch, and if that's
not possible, skip the rebase and tell the user about it. Any comments
or objections before I look into this further?
--
Magnus Bäck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson
Nasser Grainawi
I think the idea was to do revert the "easy" way because we knew it would always work. Then when someone had time they could independently add a "rebase change" function to Gerrit that could be used for any change. Once that function existed it wouldn't be hard to tie it back in with revert.
Nasser
>
> --
> Magnus Bäck Opinions are my own and do not necessarily
> SW Configuration Manager represent the ones of my employer, etc.
> Sony Ericsson
>
> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Martin Fick
> I tried the new feature for reverting changes (0908bff9,
> http://r.android.com/23019), and it's quite nice.
> However, I was surprised to find that a commit created
> by this feature has the commit being reverted as its
> parent. If you're reverting the most recent commit on a
> branch (which probably often is the case) this doesn't
> matter, but if the commit is older you may end up with
> a) a version graph that looks unnecessariyl crappy and
> b) a change that might not even merge unless it's
> rebased first.
Magnus, good concerns.
For a) It depends on your merge strategy a bit.
* If FF or CP -> then it should look just fine, no?
* If Merge, then isn't a messy history a bit sort of by
choice?
As for b) if the change cannot merge, then it likely
requires human intervention, so how can Gerrit help
without running back to a?
> It's actually quite clever to implement this by reusing
> the parent commit's tree, but I don't like the
> consequences -- if Gerrit creates a commit for me I
> expect it to be submittable or that Gerrit at least
> tells me that I'll have to rebase it myself first.
By using the parent, Gerrit is at least sure to be able to
create the revert commit. With any other strategy, that may
not be the case. As for telling you about the need for a
rebase, perhaps you will be satisfied when the submit button
gets disabled on non-mergeable changes?
> If JGit has rebase machinery built in I'd like
> ChangeUtil.revert() to rebase the created commit to the
> tip of the branch, and if that's not possible, skip the
> rebase and tell the user about it. Any comments or
> objections before I look into this further?
Unless the change is going to be applied directly by Gerrit
after the rebase, you are likely going to be back in the
same boat if your project is a busy one (new commits will
outdate it).
I think that a better way to deal with this is to create a
"rebase" button and capability that can be applied to any
patchset. Perhaps you would venture to tackle this instead?
:)
-Martin
--
Employee of Qualcomm Innovation Center, Inc. which is a
Magnus Bäck
Martin Fick <mf...@codeaurora.org> wrote:
> On Friday 22 July 2011 08:50:11 am Magnus Bäck wrote:
>
> > I tried the new feature for reverting changes (0908bff9,
> > http://r.android.com/23019), and it's quite nice. However, I was
> > surprised to find that a commit created by this feature has the
> > commit being reverted as its parent. If you're reverting the most
> > recent commit on a branch (which probably often is the case) this
> > doesn't matter, but if the commit is older you may end up with
> > a) a version graph that looks unnecessariyl crappy and b) a change
> > that might not even merge unless it's rebased first.
>
> Magnus, good concerns.
>
> For a) It depends on your merge strategy a bit.
>
> * If FF or CP -> then it should look just fine, no?
>
> * If Merge, then isn't a messy history a bit sort of by choice?
Yes, but with Gerrit's default strategy, merge if necessary, you
unvoluntarily get a crappy history -- and it's different from
how "git revert" works in all but the CP strategy case. To me,
that violates the principle of least surprise.
> As for b) if the change cannot merge, then it likely requires human
> intervention, so how can Gerrit help without running back to a?
It can help me by telling me that it's not going to work out.
Why would I want to know that? Well, I tried the new feature to get rid
of a commit that broke our build. As always, these things happen late in
the day when people have started to leave the office, but I managed to
catch a guy that would give me the necessary +2. Just before submitting
the change I happened to download it and inspect the commit with gitk,
which was when I found that the commit wasn't based on the tip of the
branch. In this particular case the commits that weren't included in
the ancestry of the revert commit didn't touch any of the files that
was modified by the revert commit so I was able to submit the change.
However, had that not been the case I would've had to rebase the
change and yet again hunt down someone that could approve. Or, more
realistically, I'd exercise my Push Branch permission ;-)
Point is, I don't want to chase down a +2 in vain when Gerrit "knows"
that my change can never be submitted unless I rebase it first. Again,
there is an inherent race condition (as always), but I'd expect Gerrit
to at least make the best of the situation.
> > It's actually quite clever to implement this by reusing the parent
> > commit's tree, but I don't like the consequences -- if Gerrit
> > creates a commit for me I expect it to be submittable or that
> > Gerrit at least tells me that I'll have to rebase it myself first.
>
> By using the parent, Gerrit is at least sure to be able to create the
> revert commit. With any other strategy, that may not be the case.
> As for telling you about the need for a rebase, perhaps you will be
> satisfied when the submit button gets disabled on non-mergeable
> changes?
Not really. In the scenario above, it's already too late when I've
tracked down a suitable approver.
> > If JGit has rebase machinery built in I'd like ChangeUtil.revert()
> > to rebase the created commit to the tip of the branch, and if that's
> > not possible, skip the rebase and tell the user about it. Any
> > comments or objections before I look into this further?
>
> Unless the change is going to be applied directly by Gerrit after the
> rebase, you are likely going to be back in the same boat if your
> project is a busy one (new commits will outdate it).
>
> I think that a better way to deal with this is to create a "rebase"
> button and capability that can be applied to any patchset. Perhaps
> you would venture to tackle this instead? :)
A general rebase feature would of course be better -- but following the
previous rationale I still want Gerrit to tell me about the necessity of
a rebase.
Thanks for you comments, Martin and Nasser. I'll poke around and see how
difficult rebasing commits would be.
Martin Fick
> On Friday, July 22, 2011 at 17:50 CEST,
>
> > > reusing the parent commit's tree, but I don't like
> > > the consequences -- if Gerrit creates a commit for me
> > > I expect it to be submittable or that Gerrit at least
> > > tells me that I'll have to rebase it myself first.
> >
> > By using the parent, Gerrit is at least sure to be able
> > to create the revert commit. With any other strategy,
> > that may not be the case. As for telling you about the
> > need for a rebase, perhaps you will be satisfied when
> > the submit button gets disabled on non-mergeable
> > changes?
>
> Not really. In the scenario above, it's already too late
> when I've tracked down a suitable approver.
Good point, I hadn't thought of that. But, that just means
that the current submit solution offered here:
https://review.source.android.com/#/c/20327/
Should be designed to be nice enough to display this
unmergeable information in another way than simply disabling
the submit button! It needs to display it directly on the
Change page even before approvals are gotten.
Would that make more sense and achieve what you want?
Magnus Bäck
Martin Fick <mf...@codeaurora.org> wrote:
Yes, that would be a great help. I often download changes just to check
there mergeability to avoid future path conflict disappointments. The
status field seems like a good candidate to display this information.
Still, I do think Gerrit should make a best effort attempt to rebase
reverted changes.</stubborn>
Gustaf Lundh
Magnus: I understand your concerns.
While I agree it would be nice to be able to Rebase a change directly
from within Gerrit, I have to agree with Martin that the Rebase
functionality should be a feature of it's own. And applicable to all
Changes and PatchSets.
The next best thing, E.g. letting the user know if the change is
Mergable directly on the change screen, has already been implemented
by our colleagues in the "Disable Submit Button"-feature (which you
probably know by now, I know I'm a bit late to the party). I would
have uploaded the new PatchSet today (including this functionality) if
our tsocks server had not been acting weird.
Lastly, there are still a few kinks relating to the "Revert change"-
feature that I plan to sort out soon. For instance, far more users
than anticipated have issues understanding how it actually works. Many
misses that a new Reviewable change is created and instead just
expects the change to be instantly reverted and merged. Therefor
leaving unhandled Revert changes in the system. This behavior could
easy be avoided by just providing the user with some clear hints. For
instance a clickable link to the new change in the change's "Revert-
message". The user should also be automatically redirected to the
newly created change after pressing the Revert-button. I plan to
tackle this as soon as possible.
Best regards
Gustaf
Magnus Bäck
Gustaf Lundh <gustaf...@sonyericsson.com> wrote:
> Magnus: I understand your concerns.
>
> While I agree it would be nice to be able to Rebase a change directly
> from within Gerrit, I have to agree with Martin that the Rebase
> functionality should be a feature of it's own. And applicable to all
> Changes and PatchSets.
Sure. I've been looking at this and it seems to be pretty straight
forward to implement. The only concern I have is that it probably
should ignore the submit type setting of the project and always use
content merging. Otherwise there would be little point in having the
feature if you could only rebase changes that don't really need to be
rebased.
> The next best thing, E.g. letting the user know if the change is
> Mergable directly on the change screen, has already been implemented
> by our colleagues in the "Disable Submit Button"-feature (which you
> probably know by now, I know I'm a bit late to the party). I would
> have uploaded the new PatchSet today (including this functionality) if
> our tsocks server had not been acting weird.
Does what you're about to upload include the comment Martin made after
this discussion, i.e. that the user should get feedback before all
approvals had been made?
> Lastly, there are still a few kinks relating to the "Revert change"-
> feature that I plan to sort out soon. For instance, far more users
> than anticipated have issues understanding how it actually works. Many
> misses that a new Reviewable change is created and instead just
> expects the change to be instantly reverted and merged. Therefor
> leaving unhandled Revert changes in the system. This behavior could
> easy be avoided by just providing the user with some clear hints. For
> instance a clickable link to the new change in the change's "Revert-
> message". The user should also be automatically redirected to the
> newly created change after pressing the Revert-button. I plan to
> tackle this as soon as possible.
Yeah, redirecting to the new revert change would make sense. I was
baffled by this when I used the feature too ("where did the revert
change go?").
Gustaf Lundh
> forward to implement.
repository?
> Does what you're about to upload include the comment Martin made after
> this discussion, i.e. that the user should get feedback before all
> approvals had been made?
Cheers
Gustaf
On Jul 29, 8:11 am, Magnus Bäck <magnus.b...@sonyericsson.com> wrote:
> On Thursday, July 28, 2011 at 16:21 CEST,
Magnus Bäck
Gustaf Lundh <gustaf...@sonyericsson.com> wrote:
> > Sure. I've been looking at this and it seems to be pretty straight
> > forward to implement.
>
> Cool. Just of curiosity; how do you carry out a rebase on a bare git
> repository?
There's an option to have JGit do in-core merges. I'll steal code from
the cherry-pick submit type code in MergeOp.java.
[...]