[RFC] Solution for duplicated or false review/bug notifications

[Jaroslaw Staniek]

On 1 November 2012 21:58, Boudewijn Rempt <boud at valdyas.org> wrote:
> On Thursday 01 November 2012 Nov, Jaroslaw Staniek wrote:
>
>> tl;dr
>> I propose to treat public KDE git branches (for all repos) having
>> '-work' suffix in a special way: ignore REVIEW and BUG/CCBUG lines.
>> When commit hits a public KDE git branch without -work suffix, current
>> behaviour is preserved.
>
> Hm...
>
>>
>> ** The problem:
>> Whenever commits are pushed from work branches and they contain
>> merged/cherry-picked commits (from other developers) with REVIEW:
>> line, we're getting repeating notes on rb and emails: "This review has
>> been submitted with commit xxxxxxxxx by yyyyyyyy to branch zzzzz."
>> I think the hook shall not generate duplicated reviews like this.
>
> But for cherry-picked commits to branches, I want to see those. And when I update my repo, then I definitely don't want to see any commits in your rebased work branch. I don't want to download them, I don't think the git hooks are the problem: the problem is those fake commits you create when rebasing a branch after merging before pushing the merge commit.
>
> You just shouldn't do that. If you merge master into a branch, push that commit before someone else commits to that branch. If you cannot, start your merge anew from that commit.
>
> If I pull from the shared repo, I don't want to see those commits.

>> Moreover the issue propagates to bugs.kde.org: multiple commits with
>> the same BUG: or CCBUG: line also cause multiple events registered
>> within the bug entry, and multiple email notifications.
>
> For cherry-picks, it's fine to see what happened. Just don't push a rebased merge. If you don't do that, the commit hooks don't need to be updated. I don't mind the hooks checking something in the branch name -- but I am not going to support changing the current calligra conventions for naming work branches. Just work in a way that suits the tools -- don't fight the way the tools work.
>

These are own KDE's tools, the original proposal is not about changing
anything in git, lists, rb or b.k.o...

> Just don't push a rebased merge.

I agree here. In general, I don't change commit hashes of anything but
the patches I am working on. (this is for example needed to fill the
REVIEW line or to update the patch itself after negative review).

Multiple 'review submitted' messages happen even if I never merge any
commits into a given work branch (and thus do not rebase any new
commits there).
You see the multiple review submissions in this scenario: I am
cherry-picking a (reviewed) commit to master branch, and, say, 2.5
branch. I cannot always perform a merge into master and 2.5 because
for example there may be another unreviewed patch but the one so far
reviewed is a proper (and only needed) addition for 2.5 (which has
tagging scheduled for tomorrow).
In general case, to be able to merge my features into release branches
I would need to have local branches per feature and per minor release
of the software. And these would be all separate builds. Cherry-pick
lets me to combine features within one build for given tests scenarios
without recompiling everything for every test I need.

One (new) idea to avoid these duplications outside of git hooks: what
if we add a condition in reviewboard itself to ignore subsequent
'submit' requests caused by duplicated REVIEW: lines? Ben?

Above notes relate to the problem marked as #2 in my original email.
But explanation of problem #1 has been stripped out in you email (I am
interested in your opinion here too):
"Once we pushed to the work branch, the review gets 'submitted' status
since there's REVIEW line - wrong - we might not get any review yet.
The same for the BUG: line."

-- 
regards / pozdrawiam, Jaroslaw Staniek
 Kexi & Calligra & KDE | http://calligra.org/kexi | http://kde.org
 Qt Certified Specialist | http://qt-project.org
 http://www.linkedin.com/in/jstaniek