Johan Corveleyn wrote on Thu, Oct 13, 2016 at 13:14:30 +0200:
> On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej <br...@apache.org> wrote:
> > On 13.10.2016 13:01, Branko Čibej wrote:
> >> On 13.10.2016 11:39, Ivan Zhakov wrote:
> >>> On 13 October 2016 at 10:59, <jcor...@apache.org> wrote:
> >>>> Author: jcorvel
> >>>> Date: Thu Oct 13 08:59:07 2016
> >>>> New Revision: 1764631
> >>>>
> >>>> URL: http://svn.apache.org/viewvc?rev=1764631&view=rev
> >>>> Log:
> >>>> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
> >>>> chance to execute the correct backport commands, after backport mess.
> >>>>
> >>> I'm wondering if we really need backport.pl running as cronjob to
> >>> merge backports automatically to the stable branches? It's not a first
> >>> time when this automatic job performs invalid merges. And as far I
> >>> understand we still spend some time to babysit this tool, fix bugs
> >>> etc. What is wrong with old proven process by merging revisions
> >>> manually?
> >> It doesn't happen all that often that backport.pl makes a mistake. I bet
> >> manual merging would be just as error-prone.
> >>
> >> Backporting is a well-defined process. The best possible way to document
> >> a process is to automate it. Errors will happen but that's no reason to
> >> revert to PEBKAC; bugs can be fixed.
> >
> >
> > In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and
> > nothing was wrong with backport.pl.
>
> Indeed, what we experienced last night was not a script bug (unless if
> you state that it should have protected against that race condition
> :-)).
I think the issue is different. backport.pl was never expected to run
concurrently with itself. However, tonight's bug would also have
occurred if a human made a commit that met all the following conditions:
- Committed between 04:00:02 and (roughly) 04:00:12 UTC,
- on a night when there are 2+ approved groups,
- removes a group (other than the last) from the "Approved changes"
section.
That's a pretty rare combination: we have few commits at around 4am UTC,
and we have few cases of vetoing an approved group. That's why this bug
has never been encountered in 5+ years of using the cron job (and even
tonight, it was encountered due to the migration, not due to a nocturnal
committer).
Patch attached.
Cheers,
Daniel
[[[
backport.py: Fix a race condition involving concurrent commits to STATUS
(see r1764633).
The fix is to run 'update' at the top of the outermost loop, rather than
immediately before calling 'svn merge'.
* tools/dist/backport/merger.py
(merge): Don't run revert+update; instead, expect the caller to have done so.
* tools/dist/merge-approved-backports.py:
Run 'update' and re-parse STATUS before each merge.
* tools/dist/detect-conflicting-backports.py:
Track API change of merge().
]]]
[[[
Index: backport/merger.py
===================================================================
--- backport/merger.py (revision 1762016)
+++ backport/merger.py (working copy)
@@ -195,11 +195,8 @@ def merge(entry, expected_stderr=None, *, commit=F
mergeargs.extend(['--', sf.trunk_url()])
logmsg += entry.raw
- # TODO(interactive mode): exclude STATUS from reverts
- # TODO(interactive mode): save local mods to disk, as backport.pl does
- run_revert()
+ no_local_mods('.')
- run_svn_quiet(['update'])
# TODO: use select() to restore interweaving of stdout/stderr
_, stdout, stderr = run_svn_quiet(['merge'] + mergeargs, expected_stderr)
sys.stdout.write(stdout)
Index: detect-conflicting-backports.py
===================================================================
--- detect-conflicting-backports.py (revision 1762016)
+++ detect-conflicting-backports.py (working copy)
@@ -77,6 +77,7 @@ ERRORS = collections.defaultdict(list)
for entry_para in sf.entries_paras():
entry = entry_para.entry()
# SVN_ERR_WC_FOUND_CONFLICT = 155015
+ backport.merger.run_svn_quiet(['update']) # TODO: what to do if this pulls
in a STATUS mod?
backport.merger.merge(entry, 'svn: E155015' if entry.depends else None)
_, output, _ = backport.merger.run_svn(['status'])
Index: merge-approved-backports.py
===================================================================
--- merge-approved-backports.py (revision 1762016)
+++ merge-approved-backports.py (working copy)
@@ -40,11 +40,14 @@ if sys.argv[1:]:
sys.exit(0)
backport.merger.no_local_mods('./STATUS')
-sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8"))
-# Duplicate sf.paragraphs, since merge() will be removing elements from it.
-entries_paras = list(sf.entries_paras())
-for entry_para in entries_paras:
- if entry_para.approved():
- entry = entry_para.entry()
- backport.merger.merge(entry, commit=True)
+while True:
+ backport.merger.run_svn_quiet(['update'])
+ sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8"))
+ for entry_para in sf.entries_paras():
+ if entry_para.approved():
+ entry = entry_para.entry()
+ backport.merger.merge(entry, commit=True)
+ break # 'continue' the outer loop
+ else:
+ break
]]]
