Rebase on behalf of the uploader - experimental feature

[Edwin Kempin]

Hi Gerrit Users,

We are testing rebase on behalf of the uploader on the gerrit host.

TLDR:

From now on, rebasing a change from the web UI preserves the uploader. This means a reviewer can rebase a change and then continue with approving and submitting. 

Long version:

If you rebase a change (or a chain of changes) from the web UI the rebase will be done on behalf of the uploader now. This means that the uploader stays intact (the uploader of the new patch set is the same as the uploader of the patch set that was rebased).

This has the advantage that the rebaser is not taking over the change, which is important when the project is configured to ignore self-approvals of the uploader, as this allows the reviewer to approve and submit the change, after doing a rebase. With a normal rebase the rebaser becomes the uploader and submitting the change requires an approval of another user (e.g. from the original uploader). Getting the approval of another user is an unnecessary review round and delays submitting the change. 

Since rebasing on behalf of the uploader avoids unnecessary review rounds it is always done for rebasing changes from the web UI now.

Rebasing on behalf of the uploader is only allowed for trivial rebases so that it's safe to assume an implicit approval of the uploader (the diff of the rebase patch set is exactly the same diff that was originally uploaded). This means it's not possible to rebase on behalf of the uploader when the  "Allow rebase with conflicts" option is selected (as it's not a trivial rebase when conflicts need to be resolved). This means if the "Allow rebase with conflicts" option is selected the rebase is still done on behalf of the user doing the rebase. To make users aware of this the web UI shows the following warning if the "Allow rebase with conflicts" option is selected:

If a rebase is done on behalf of the uploader, the actual user doing the rebase is recorded in the change metadata, so that from the change metadata we can always tell who was doing the rebase. In the web UI the actual rebaser can be seen from the message that is posted on rebase:

Reach out to us!

• Ask a question or give us some feedback: repo-d...@googlegroups.com

• File a Bug or Feature Request: https://bugs.chromium.org/p/gerrit/issues/entry?template=GoogleSource+Issue

- Edwin & Milutin on behalf of the Gerrit Code Review team

[Luca Milanesio]

On 8 Mar 2023, at 15:46, 'Edwin Kempin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Hi Gerrit Users,

We are testing rebase on behalf of the uploader on the gerrit host.

TLDR:

From now on, rebasing a change from the web UI preserves the uploader. This means a reviewer can rebase a change and then continue with approving and submitting. 

Long version:

If you rebase a change (or a chain of changes) from the web UI the rebase will be done on behalf of the uploader now. This means that the uploader stays intact (the uploader of the new patch set is the same as the uploader of the patch set that was rebased).

This has the advantage that the rebaser is not taking over the change, which is important when the project is configured to ignore self-approvals of the uploader, as this allows the reviewer to approve and submit the change, after doing a rebase. With a normal rebase the rebaser becomes the uploader and submitting the change requires an approval of another user (e.g. from the original uploader). Getting the approval of another user is an unnecessary review round and delays submitting the change. 

Thanks a lot for this feature Edwin, 

That would help speeding up reviews and merge.

How many times I’ve rebased and made a change non-mergeable anymore :-( this feature definitely helps me a lot !

Luca.

Since rebasing on behalf of the uploader avoids unnecessary review rounds it is always done for rebasing changes from the web UI now.

Rebasing on behalf of the uploader is only allowed for trivial rebases so that it's safe to assume an implicit approval of the uploader (the diff of the rebase patch set is exactly the same diff that was originally uploaded). This means it's not possible to rebase on behalf of the uploader when the  "Allow rebase with conflicts" option is selected (as it's not a trivial rebase when conflicts need to be resolved). This means if the "Allow rebase with conflicts" option is selected the rebase is still done on behalf of the user doing the rebase. To make users aware of this the web UI shows the following warning if the "Allow rebase with conflicts" option is selected:

If a rebase is done on behalf of the uploader, the actual user doing the rebase is recorded in the change metadata, so that from the change metadata we can always tell who was doing the rebase. In the web UI the actual rebaser can be seen from the message that is posted on rebase:

Reach out to us!

• Ask a question or give us some feedback: repo-d...@googlegroups.com

• File a Bug or Feature Request: https://bugs.chromium.org/p/gerrit/issues/entry?template=GoogleSource+Issue

- Edwin & Milutin on behalf of the Gerrit Code Review team

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

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CACA_R4L8uM0aDcHBscV1hcFVdDi8P%3Djs9rHG370wsHgc%2BEj52g%40mail.gmail.com.

[Nasser Grainawi]

On Wed, Mar 8, 2023 at 1:56 PM Luca Milanesio <luca.mi...@gmail.com> wrote:

On 8 Mar 2023, at 15:46, 'Edwin Kempin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Hi Gerrit Users,

We are testing rebase on behalf of the uploader on the gerrit host.

TLDR:

From now on, rebasing a change from the web UI preserves the uploader. This means a reviewer can rebase a change and then continue with approving and submitting. 

Long version:

If you rebase a change (or a chain of changes) from the web UI the rebase will be done on behalf of the uploader now. This means that the uploader stays intact (the uploader of the new patch set is the same as the uploader of the patch set that was rebased).

This has the advantage that the rebaser is not taking over the change, which is important when the project is configured to ignore self-approvals of the uploader, as this allows the reviewer to approve and submit the change, after doing a rebase. With a normal rebase the rebaser becomes the uploader and submitting the change requires an approval of another user (e.g. from the original uploader). Getting the approval of another user is an unnecessary review round and delays submitting the change. 

Thanks a lot for this feature Edwin, 

That would help speeding up reviews and merge.

How many times I’ve rebased and made a change non-mergeable anymore :-( this feature definitely helps me a lot !

I've done that as well, but I wonder if there's a better approach that fixes that problem and preserves the git-like rebase behavior. Since rebase-as-uploader only works for the trivial rebase case, could the submit requirement consider uploader self-approval as valid when the current patchset is a trivial rebase and the most recent non-trivial-rebase patchset has a different uploader? A simpler starting point would even be to only look back 1 previous patchset since that's probably the most common case. @eke...@google.com thoughts?

 

To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/9F4F1123-6546-4CAD-BF42-D7F71329BE22%40gmail.com.

[Edwin Kempin]

On Sat, Apr 1, 2023 at 1:26 AM Nasser Grainawi <nasser....@linaro.org> wrote:

On Wed, Mar 8, 2023 at 1:56 PM Luca Milanesio <luca.mi...@gmail.com> wrote:

On 8 Mar 2023, at 15:46, 'Edwin Kempin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Hi Gerrit Users,

We are testing rebase on behalf of the uploader on the gerrit host.

TLDR:

From now on, rebasing a change from the web UI preserves the uploader. This means a reviewer can rebase a change and then continue with approving and submitting. 

Long version:

If you rebase a change (or a chain of changes) from the web UI the rebase will be done on behalf of the uploader now. This means that the uploader stays intact (the uploader of the new patch set is the same as the uploader of the patch set that was rebased).

This has the advantage that the rebaser is not taking over the change, which is important when the project is configured to ignore self-approvals of the uploader, as this allows the reviewer to approve and submit the change, after doing a rebase. With a normal rebase the rebaser becomes the uploader and submitting the change requires an approval of another user (e.g. from the original uploader). Getting the approval of another user is an unnecessary review round and delays submitting the change. 

Thanks a lot for this feature Edwin, 

That would help speeding up reviews and merge.

How many times I’ve rebased and made a change non-mergeable anymore :-( this feature definitely helps me a lot !

I've done that as well, but I wonder if there's a better approach that fixes that problem and preserves the git-like rebase behavior. Since rebase-as-uploader only works for the trivial rebase case,

It's intentionally only working for trivial rebases, it's not a technical limitation.

 

could the submit requirement consider uploader self-approval as valid when the current patchset is a trivial rebase and the most recent non-trivial-rebase patchset has a different uploader?

I considered this but discarded this idea for the following reasons:

* This would require the submit requirement to know after the fact whether an upload was a trivial rebase:

   - It could detect this by redoing a rebase and check whether the result matches the actual patch set. I think doing this is more complicated than supporting rebase on behalf of the uploader. Also it's expensive which is a problem since submit requirements are evaluated each time a change is updated (for reindex).

   - We could record the change kind in NoteDb (this might still be a good idea for other reasons). This would require computing/retrieving the change kind in new places (at the moment we only use it for approval copying). Doing this might go at the expense of the write latency (assuming we can do approval copying async at some point in time).

* You would need to express the condition as a predicate in a way that's understandable by users. If we would extend the existing "label:Code-Review=MAX,user=non_uploader" syntax we would end up with something like "label:Code-Review=MAX,user=non_uploader_of_last_non_trivial_rebase_patch_set" which is not easy to grasp and likely only creates confusion.

* If you want this globally you would need to include it into each Code-Review submit requirement.

* The new predicate would be the first one which does not only look at the current change state (aka the latest patch set) but that would also need to look at outdated patch sets, this makes this predicate kind of inconsistent with those that we have so far.

 

A simpler starting point would even be to only look back 1 previous patchset since that's probably the most common case. @eke...@google.com thoughts?

This might be simpler, but it would be confusing for users that it stops working if you rebase a change multiple times in a row.

What I like about rebasing on behalf of the uploader also is that it's consistent with the existing support for voting and submitting on behalf of.

FWIW we have fully rolled this out and users are happy with it. Milutin is still making some final UX improvements so that from the rebase dialog it's more clear who will be the uploader.

[Nasser Grainawi]

On Mon, Apr 3, 2023 at 1:15 AM Edwin Kempin <eke...@google.com> wrote:

On Sat, Apr 1, 2023 at 1:26 AM Nasser Grainawi <nasser....@linaro.org> wrote:

On Wed, Mar 8, 2023 at 1:56 PM Luca Milanesio <luca.mi...@gmail.com> wrote:

On 8 Mar 2023, at 15:46, 'Edwin Kempin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Hi Gerrit Users,

We are testing rebase on behalf of the uploader on the gerrit host.

TLDR:

From now on, rebasing a change from the web UI preserves the uploader. This means a reviewer can rebase a change and then continue with approving and submitting. 

Long version:

If you rebase a change (or a chain of changes) from the web UI the rebase will be done on behalf of the uploader now. This means that the uploader stays intact (the uploader of the new patch set is the same as the uploader of the patch set that was rebased).

This has the advantage that the rebaser is not taking over the change, which is important when the project is configured to ignore self-approvals of the uploader, as this allows the reviewer to approve and submit the change, after doing a rebase. With a normal rebase the rebaser becomes the uploader and submitting the change requires an approval of another user (e.g. from the original uploader). Getting the approval of another user is an unnecessary review round and delays submitting the change. 

Thanks a lot for this feature Edwin, 

That would help speeding up reviews and merge.

How many times I’ve rebased and made a change non-mergeable anymore :-( this feature definitely helps me a lot !

I've done that as well, but I wonder if there's a better approach that fixes that problem and preserves the git-like rebase behavior. Since rebase-as-uploader only works for the trivial rebase case,

It's intentionally only working for trivial rebases, it's not a technical limitation.

 

could the submit requirement consider uploader self-approval as valid when the current patchset is a trivial rebase and the most recent non-trivial-rebase patchset has a different uploader?

I considered this but discarded this idea for the following reasons:

* This would require the submit requirement to know after the fact whether an upload was a trivial rebase:

   - It could detect this by redoing a rebase and check whether the result matches the actual patch set. I think doing this is more complicated than supporting rebase on behalf of the uploader. Also it's expensive which is a problem since submit requirements are evaluated each time a change is updated (for reindex).

   - We could record the change kind in NoteDb (this might still be a good idea for other reasons). This would require computing/retrieving the change kind in new places (at the moment we only use it for approval copying). Doing this might go at the expense of the write latency (assuming we can do approval copying async at some point in time).

* You would need to express the condition as a predicate in a way that's understandable by users. If we would extend the existing "label:Code-Review=MAX,user=non_uploader" syntax we would end up with something like "label:Code-Review=MAX,user=non_uploader_of_last_non_trivial_rebase_patch_set" which is not easy to grasp and likely only creates confusion.

* If you want this globally you would need to include it into each Code-Review submit requirement.

* The new predicate would be the first one which does not only look at the current change state (aka the latest patch set) but that would also need to look at outdated patch sets, this makes this predicate kind of inconsistent with those that we have so far.

Thanks for summarizing those! They seem like some tricky challenges and rebasing on behalf of the uploader does seem like a nice way to avoid those problems. Maybe we'll need to solve them for some other reason in the future so it's great that we have some of your thoughts on them recorded here now.

 

 

A simpler starting point would even be to only look back 1 previous patchset since that's probably the most common case. @eke...@google.com thoughts?

This might be simpler, but it would be confusing for users that it stops working if you rebase a change multiple times in a row.

What I like about rebasing on behalf of the uploader also is that it's consistent with the existing support for voting and submitting on behalf of.

FWIW we have fully rolled this out and users are happy with it. Milutin is still making some final UX improvements so that from the rebase dialog it's more clear who will be the uploader.

+1