Download it now!
Bug 79123 - Autoabandon gerrit changes with negative feedback and more than 1 month without action
Summary: Autoabandon gerrit changes with negative feedback and more than 1 month witho...
Status: RESOLVED MOVED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: ci-infra (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillScript, topicWeb
Depends on:
Blocks:
 
Reported: 2014-05-23 14:11 UTC by Björn Michaelsen
Modified: 2016-02-18 16:37 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Michaelsen 2014-05-23 14:11:05 UTC
It would be nice to have a cronjob auto abandon changes with a negative review and no update for more than a month.

Here is a query for such changes:

 https://gerrit.libreoffice.org/#/q/status:open+%28label:Code-Review%253D-1+OR+label:Code-Review%253D-2+OR+label:Verified%253D-1%29+age:1month,n,z

The script sending the daily digest to the developers might be used as inspiration:

 https://gerrit.libreoffice.org/gitweb?p=dev-tools.git;a=blob;f=gerritbot/send-daily-digest;h=79d86b8926ec7be8a5b9624e20e06d5c6275d85e;hb=99053ffcc1c246d9dddc8d42c1bd447dbcb196f5


see http://nabble.documentfoundation.org/minutes-of-ESC-call-td4109744.html for reference on the discussion on the ESC
Comment 1 Norbert Thiebaud 2014-05-23 15:28:22 UTC
exception: if a patch has been marked with negative feedback by the author of the patch himself, leave it alone
marking one's patch with -2 is used to indicate a 'working' patch.. something uploaded for review/discussion purpose but not intended to be merge as is
There is no reason to make these disappear
Comment 2 Norbert Thiebaud 2014-05-23 15:30:14 UTC
Another things: that auto purge stuff _have to_ be project configurable...
gerrit is used to track MC activity for instance, and patch there must not be messed with in any way.
Comment 3 Björn Michaelsen 2014-05-23 15:42:41 UTC
(In reply to comment #1)
> exception: if a patch has been marked with negative feedback by the author
> of the patch himself, leave it alone
> marking one's patch with -2 is used to indicate a 'working' patch..

No even then. The recommended way to do a working patch is using drafts:

 https://wiki.documentfoundation.org/Gerrit#Submitting_patches_as_drafts

Everything else is frustrating reviewers checking "open" changes.

Also, even _if_ a change would still be uploaded accidentally for review although it is not intended to be merged as is, there is little harm done, when the change is abandoned under the conditions above. Reviving a abandoned change is trivial and the autoabandon is actually a helpful reminder -- better than letting them stay in limbo for months.
Comment 4 Björn Michaelsen 2014-05-23 15:43:49 UTC
(In reply to comment #2)
> Another things: that auto purge stuff _have to_ be project configurable...
> gerrit is used to track MC activity for instance, and patch there must not
> be messed with in any way.

As an Easy Hack is limited to project:core for the start -- its the majority of changes anyway.
Comment 5 Norbert Thiebaud 2014-05-23 16:16:22 UTC
"No even then. The recommended way to do a working patch is using drafts:"

I disagree with that.. quoting yourself from a wiki page does not make a 'recommended' way.
Draft are a pain in the but... and when you wrote that it was not even possible to view other's draft for most people
So the feature exist.. but is by no mean 'recommended'.

"there is little harm done, when the change is abandoned under the conditions above. Reviving a abandoned change is trivial and the autoabandon is actually a helpful reminder -- better than letting them stay in limbo for months."

Patch are presented typically from the newsest modified to the oldest modified.
furthermore reviewed -2 patch are visually easy to detect
so ... such patch do not actually interfere with normal reviewer works
I do not see the ground to put the burden of 'little harm' to fellow committers by messing with their work just to make the main page 'look good'

I'm not arguing against the clean-up of drive by patch not being worked upon.. but a patch uploaded by a committer, explicitly marked -2 by him should be left alone.
Comment 6 Björn Michaelsen 2014-05-23 17:43:22 UTC
(In reply to comment #5)
> I'm not arguing against the clean-up of drive by patch not being worked
> upon.. but a patch uploaded by a committer, explicitly marked -2 by him
> should be left alone.

Not if its untouched for more than a month.
Comment 7 Norbert Thiebaud 2014-05-23 20:44:56 UTC
"Not if its untouched for more than a month."

We do not all work fulltime on LibreOffice... a month maybe seem overly long to you.. but it is only 4 week-end...
In any case what is the goal here ? If I mark a patch -2 myself, that means it is not ready, and it also mean I'm aware of it and will follow up on it... or abandon it myself if need be. Why would you want to mess with that ? what is the benefit ?
The benefit from my side is the ability, for instance, to to buildbot build on an experimental thing... thing that may takes months of amateur tinkering before being in shape for merging.
Comment 8 Björn Michaelsen 2014-05-23 22:02:22 UTC
(In reply to comment #7)
> In any case what is the goal here ? If I mark a patch -2 myself, that means
> it is not ready, and it also mean I'm aware of it and will follow up on
> it... or abandon it myself if need be. Why would you want to mess with that
> ? what is the benefit ?

This is to prevent reviewer to run into the same unchanged patches again and again when checking on janitorial patrol. Frustrating reviewer is not something we should do as they are rare anyway.

As said trying to get feedback on a self-inflicted -2 change is suboptimal anyway as a/ there are drafts for that b/ many reviewers will not even open a change marked as -2 on the overview, esp. when that change is a few weeks old

The auto-abandon stops frustrating those few reviewer that do walk _all_ change (which is a good thing) and it reminds the submitters that they still have something open and it hurts the change in no way. As a casual submitter to openstack, I found the autoabandon very helpful and not at all discouraging -- rather it promotes engagement.
Comment 10 Robinson Tryon (qubit) 2015-12-15 23:21:50 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillScript TopicWeb)
[NinjaEdit]
Comment 11 Robinson Tryon (qubit) 2016-02-18 16:37:23 UTC
Remove LibreOffice Dev List from CC on EasyHacks
(curtailing excessive email to list)
[NinjaEdit]