'Old Version History' & Rebased Changes

[kai.k...@nokia.com]

Hi,

One thing that I often run into is this:

- Reviewer approves Patch Set 1
- Patch Set 1 does not merge due to path conflicts
- Author rebases commit, re-submits
- Reviewer is asked to review Patch Set 2, and wonders whether the author has changed anything except rebasing

Now we just updated to 2.1.7 just today, and I had high hopes that 'Old Version History' feature would help in this situation. Anyhow, it shows all the files that are changed not only in the commit, but also the rebased history. Or did I miss something?

I've read through the discussions about Rietveld in the 'Why does Gerrit send so emails' thread, but am not sure whether Rietveld would help in this situation.

Regards

Kai Koehne

--
Kai Koehne
Software Engineer
Nokia, Qt Development Frameworks
 
Nokia gate5 GmbH
Firmensitz: Invalidenstr. 117, 10115 Berlin, Germany
Registergericht: Amtsgericht Charlottenburg, Berlin: HRB 106443 B
Umsatzsteueridentifikationsnummer: DE 812 845 193
Geschäftsführer: Dr. Michael Halbherr, Karim Tähtivuori

[Shawn Pearce]

On Wed, Jun 22, 2011 at 01:58, <kai.k...@nokia.com> wrote:
> One thing that I often run into is this:
>
> - Reviewer approves Patch Set 1
> - Patch Set 1 does not merge due to path conflicts
> - Author rebases commit, re-submits
> - Reviewer is asked to review Patch Set 2, and wonders whether the author has changed anything except rebasing
>
> Now we just updated to 2.1.7 just today, and I had high hopes that 'Old Version History' feature would help in this situation. Anyhow, it shows all the files that are changed not only in the commit, but also the rebased history. Or did I miss something?

Nope, we didn't fix this yet. We have an idea of how to implement
correcting for rebase, but nobody has attempted writing the code for
it yet.

> I've read through the discussions about Rietveld in the 'Why does Gerrit send so emails' thread, but am not sure whether Rietveld would help in this situation.

Nope, it wouldn't. Reitveld doesn't understand the notion of
"rebasing" a patch set. At least the last time I looked it didn't.
Maybe Retiveld is able to work around it somehow by having the patch
set differences only difference the regions that are edited by either
patch set?

[Ishaaq Chandy]

I have a related problem - a changeset that is a merge commit between two branches shows up on the Gerrit UI as an empty changeset, despite the fact that, in fact, there were changes in the commit, for e.g. merge conflict resolution, code that had to be changed due to compilation errors that arose due to the merge, etc. Of course, the actual commit is preserved correctly and can be pulled as it was originally, its just the UI seems to indicate that nothing was changed as a result of the merge.

Ishaaq

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Shawn Pearce]

On Wed, Jun 22, 2011 at 16:30, Ishaaq Chandy <ish...@gmail.com> wrote:
> I have a related problem - a changeset that is a merge commit between two
> branches shows up on the Gerrit UI as an empty changeset, despite the fact
> that, in fact, there were changes in the commit, for e.g. merge conflict
> resolution, code that had to be changed due to compilation errors that arose
> due to the merge, etc. Of course, the actual commit is preserved correctly
> and can be pulled as it was originally, its just the UI seems to indicate
> that nothing was changed as a result of the merge.

Known bug/mis-feature. Gerrit doesn't show anything on merge commits.

I want to change that to have Gerrit compute an auto-merge, and then
show how the commit differs from the auto-merge result Gerrit
computed. If there were conflicts, I'll have the auto-merge result
include conflict markers, so reviewers can see what the conflict was
and compare that to how the result was created.

This isn't that hard anymore. The only problem is the auto-merge
result needs to be written out to the Git repository so the current
code can read it back in and do the compare to the commit. Since that
auto-merge result commit isn't actually relevant for anything, it will
be considered garbage by Git and can be removed by GC at any time.
However we want to make sure its valid before trying to read one of
those objects during any page view... and clearly we can recompute it
if an object was removed.

I'm sort of leaning towards making a single garbage reference under
refs/changes, like refs/changes/gerrit-cache, and chaining these
auto-merge results onto that branch just to keep them from being
removed by GC. An administrator could kill this branch (and thus force
Gerrit to recompute things as necessary again), but could only do that
safely while the server wasn't actually running.

Hmm. Maybe I'll try to take a stab at this tomorrow. It doesn't sound
that hard anymore now that I've invented this concept of
refs/changes/gerrit-cache.

[Jay Soffian]

On Wed, Jun 22, 2011 at 7:30 PM, Ishaaq Chandy <ish...@gmail.com> wrote:
> I have a related problem - a changeset that is a merge commit between two
> branches shows up on the Gerrit UI as an empty changeset, despite the fact
> that, in fact, there were changes in the commit, for e.g. merge conflict
> resolution, code that had to be changed due to compilation errors that arose
> due to the merge, etc. Of course, the actual commit is preserved correctly
> and can be pulled as it was originally, its just the UI seems to indicate
> that nothing was changed as a result of the merge.

Gerrit hasn't supported showing diffs for merge commits since it
switched to generating diffs internally:

http://code.google.com/p/gerrit/issues/detail?id=665

j.

[Ishaaq Chandy]

Awesome! This has been bugging me for a while now. Thanks

[Jean-Baptiste Queru]

Along the same lines, it'd be great if Gerrit could show the predicted
diff between the post-merge state and the pre-merge state of the
target branch.

Yup, I realize that it's going to change over time.

That'd make it easier to spot situations where people rebase or upload
against the wrong branch.

JBQ

> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
>

--
Jean-Baptiste M. "JBQ" Queru
Software Engineer, Android Open-Source Project, Google.

Questions sent directly to me that have no reason for being private
will likely get ignored or forwarded to a public forum with no further
warning.

[Simon Que]

Is this still an issue on anyone's plate?  It seems like it was not closed, but there hasn't been anything on it for months.

[Martin Fick]

I don't believe anyone is currently working on this.

Simon Que <sq...@google.com> wrote:

Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum

[Lundh, Gustaf]

I know Bruce have a patch related to the "Comparing rebased
PatchSets"-issue. It would not solve the rebase issue completely,
however it helps making the situation a bit more easy to handle.

The patch makes it easier to spot what have actually changed
between rebased PatchSets on a filename level. It does so by simply
not listing the files that was not touched in any of the PatchSets
you are currently comparing (e.g. when compared individually to each
PatchSet's respective parent).

At least we would get rid of the listings of "1000s of files"
that sometimes appear when you compare a newly rebased change to
an older one (especially on an active and large project).

Also. I have just picked up Magnus Bäck old feature, the
Rebase-button on the ChangeScreen. It still needs some
polishing and fixes, but I hope we can finish it up quite
soon.

Cheers
Gustaf

Simon Que <sq...@google.com> wrote:

--

[Swindells, Thomas]

Both of these would be great addition - the rebase mess particularly is a right pain when trying to review changes on a mid-large size project.

Thomas

**************************************************************************************
This message is confidential and intended only for the addressee. If you have received this message in error, please immediately notify the postm...@nds.com and delete it from your system as well as any copies. The content of e-mails as well as traffic data may be monitored by NDS for employment and security purposes. To protect the environment please do not print this e-mail unless necessary.

NDS Limited. Registered Office: One London Road, Staines, Middlesex, TW18 4EX, United Kingdom. A company registered in England and Wales. Registered no. 3080780. VAT no. GB 603 8808 40-00
**************************************************************************************

[Magnus Bäck]

On Thursday, January 26, 2012 at 10:40 CET,
"Lundh, Gustaf" <Gustaf...@sonyericsson.com> wrote:

[...]

> Also. I have just picked up Magnus Bäck old feature, the Rebase-button
> on the ChangeScreen. It still needs some polishing and fixes, but I
> hope we can finish it up quite soon.

Yeah, polishing it so that it actually compiles, for example ;-) Sorry
for not working on it for months. Since October I've had to dedicate all
my spare time to other stuff. I had hoped to be able to work on the
patch during last week and this week, but it just hasn't been possible.

--
Magnus Bäck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson