From f6f46cd221e35bf80d3b7c24ec4b4d799446399f Mon Sep 17 00:00:00 2001 From: Konstantin Ryabitsev Date: Mon, 17 May 2021 12:04:09 -0400 Subject: Implement partial reroll It has been a common request to support partial series rerolls where someone sends an amended patch as a follow-up to a previous series, e.g.: [PATCH v3 1/3] Patch one [PATCH v3 2/3] Patch two \- Re: [PATCH v3 2/3] Patch two Looks good, but please fix this $small_thing \- [PATCH v4 2/3] Patch two [PATCH v3] Patch three Previously, b4 refused to consider v4 as a complete new series, but now it will properly perform a partial reroll, but only in the cases where such patches are sent as follow-ups to the exact same patch number in the previous series: [PATCH v3->v4 1/3] Patch one [PATCH v4 2/3] Patch two [PATCH v3->v4 3/3] Patch three Reported-by: Dan Williams Signed-off-by: Konstantin Ryabitsev Link: https://lore.kernel.org/r/CAPcyv4ggbuHbqKV33_TpE7pqxvRag34baJrX3yQe-jXOikoATQ@mail.gmail.com --- b4/__init__.py | 143 ++++++++++++++++++++++++++++++++++++++++++--------------- b4/command.py | 4 +- b4/diff.py | 8 +++- b4/mbox.py | 10 +++- 4 files changed, 124 insertions(+), 41 deletions(-) diff --git a/b4/__init__.py b/b4/__init__.py index 11ad012..1418641 100644 --- a/b4/__init__.py +++ b/b4/__init__.py @@ -15,6 +15,7 @@ import requests import urllib.parse import datetime import time +import copy import shutil import mailbox # noinspection PyCompatibility @@ -229,7 +230,66 @@ class LoreMailbox: logger.info('Successfully backfilled missing patches') break - def get_series(self, revision=None, sloppytrailers=False, backfill=True): + def partial_reroll(self, revision, sloppytrailers, backfill): + # Is it a partial reroll? + # To qualify for a partial reroll: + # 1. Needs to be version > 1 + # 2. Replies need to be to the exact X/N of the previous revision + if revision <= 1 or revision - 1 not in self.series: + return + # Are existing patches replies to previous revisions with the same counter? + pser = self.get_series(revision-1, sloppytrailers=sloppytrailers, backfill=backfill) + lser = self.series[revision] + sane = True + for patch in lser.patches: + if patch is None: + continue + if patch.in_reply_to is None or patch.in_reply_to not in self.msgid_map: + logger.debug('Patch not sent as a reply-to') + sane = False + break + ppatch = self.msgid_map[patch.in_reply_to] + found = False + while True: + if patch.counter == ppatch.counter and patch.expected == ppatch.expected: + logger.debug('Found a previous matching patch in v%s', ppatch.revision) + found = True + break + # Do we have another level up? + if ppatch.in_reply_to is None or ppatch.in_reply_to not in self.msgid_map: + break + ppatch = self.msgid_map[ppatch.in_reply_to] + + if not found: + sane = False + logger.debug('Patch not a reply to a patch with the same counter/expected (%s/%s != %s/%s)', + patch.counter, patch.expected, ppatch.counter, ppatch.expected) + break + + if not sane: + logger.debug('Not a sane partial reroll') + return + logger.info('Partial reroll detected, reconstituting from v%s', pser.revision) + logger.debug('Reconstituting a partial reroll') + at = 0 + for patch in lser.patches: + if patch is None: + ppatch = copy.deepcopy(pser.patches[at]) + ppatch.revision = lser.revision + ppatch.reroll_from_revision = pser.revision + lser.patches[at] = ppatch + else: + patch.reroll_from_revision = lser.revision + at += 1 + if None not in lser.patches[1:]: + lser.complete = True + lser.partial_reroll = True + if lser.patches[0] is not None: + lser.has_cover = True + lser.subject = pser.subject + logger.debug('Reconstituted successfully') + + def get_series(self, revision=None, sloppytrailers=False, backfill=True, reroll=True): if revision is None: if not len(self.series): return None @@ -250,11 +310,14 @@ class LoreMailbox: logger.critical('All patches in series v%s are missing.', lser.revision) return None + if not lser.complete and reroll: + self.partial_reroll(revision, sloppytrailers, backfill) + if not lser.complete and backfill: self.backfill(revision) # Grab our cover letter if we have one - if revision in self.covers.keys(): + if revision in self.covers: lser.add_patch(self.covers[revision]) lser.has_cover = True else: @@ -307,18 +370,15 @@ class LoreMailbox: for trailer in trailers: logger.debug('%s%s: %s', ' ' * (lvl+1), trailer[0], trailer[1]) if pmsg.has_diff and not pmsg.reply: - # TODO: See below # We found the patch for these trailers - # if pmsg.revision != revision: - # # add this into our trailer map to carry over trailers from - # # previous revisions to current revision if patch/metadata did - # # not change - # pmsg.load_hashes() - # if pmsg.attestation: - # attid = pmsg.attestation.attid - # if attid not in self.trailer_map: - # self.trailer_map[attid] = list() - # self.trailer_map[attid] += trailers + if pmsg.revision != revision: + # add this into our trailer map to carry over trailers from + # previous revisions to current revision if patch id did + # not change + if pmsg.pwhash: + if pmsg.pwhash not in self.trailer_map: + self.trailer_map[pmsg.pwhash] = list() + self.trailer_map[pmsg.pwhash] += trailers pmsg.followup_trailers += trailers break if not pmsg.reply: @@ -334,13 +394,11 @@ class LoreMailbox: break # Carry over trailers from previous series if patch/metadata did not change - # TODO: This needs fixing, because we no longer generate the three hashes - # TODO: and patchwork_hash only gives us the hash of the actual patch - # for lmsg in lser.patches: - # if lmsg is None or lmsg.patchwork_hash is None: - # continue - # if lmsg.patchwork_hash in self.trailer_map: - # lmsg.followup_trailers += self.trailer_map[lmsg.patchwork_hash] + for lmsg in lser.patches: + if lmsg is None or lmsg.pwhash is None: + continue + if lmsg.pwhash in self.trailer_map: + lmsg.followup_trailers += self.trailer_map[lmsg.pwhash] return lser @@ -415,6 +473,7 @@ class LoreSeries: self.trailer_mismatches = set() self.complete = False self.has_cover = False + self.partial_reroll = False self.subject = '(untitled)' def __repr__(self): @@ -424,13 +483,12 @@ class LoreSeries: out.append(' expected: %s' % self.expected) out.append(' complete: %s' % self.complete) out.append(' has_cover: %s' % self.has_cover) + out.append(' partial_reroll: %s' % self.partial_reroll) out.append(' patches:') at = 0 for member in self.patches: if member is not None: out.append(' [%s/%s] %s' % (at, self.expected, member.subject)) - if member.followup_trailers: - out.append(' Add: %s' % ', '.join(member.followup_trailers)) else: out.append(' [%s/%s] MISSING' % (at, self.expected)) at += 1 @@ -547,13 +605,13 @@ class LoreSeries: if attsame and not attcrit: if attmark: - logger.info(' %s %s', attmark, lmsg.full_subject) + logger.info(' %s %s', attmark, lmsg.get_am_subject()) else: - logger.info(' %s', lmsg.full_subject) + logger.info(' %s', lmsg.get_am_subject()) else: checkmark, trailers, critical = lmsg.get_attestation_trailers(attpolicy, maxdays) - logger.info(' %s %s', checkmark, lmsg.full_subject) + logger.info(' %s %s', checkmark, lmsg.get_am_subject()) for trailer in trailers: logger.info(' %s', trailer) @@ -647,7 +705,7 @@ class LoreSeries: logger.critical('Cannot operate on an empty series') return None, None cachedata = get_cache(msgid, suffix='fakeam') - if cachedata: + if cachedata and not self.partial_reroll: stalecache = False chunks = cachedata.strip().split() if len(chunks) == 2: @@ -760,6 +818,7 @@ class LoreMessage: self.subject = None self.reply = False self.revision = 1 + self.reroll_from_revision = None self.counter = 1 self.expected = 1 self.revision_inferred = True @@ -1403,6 +1462,24 @@ class LoreMessage: self.body += signature self.body += '\n' + def get_am_subject(self): + # Return a clean patch subject + parts = ['PATCH'] + if self.lsubject.rfc: + parts.append('RFC') + if not self.revision_inferred: + if self.reroll_from_revision: + if self.reroll_from_revision != self.revision: + parts.append('v%d->v%d' % (self.reroll_from_revision, self.revision)) + else: + parts.append(' %s v%d' % (' ' * len(str(self.reroll_from_revision)), self.revision)) + else: + parts.append('v%d' % self.revision) + if not self.lsubject.counters_inferred: + parts.append('%d/%d' % (self.lsubject.counter, self.lsubject.expected)) + + return '[%s] %s' % (' '.join(parts), self.lsubject.subject) + def get_am_message(self, add_trailers=True, trailer_order=None, copyccs=False): if add_trailers: self.fix_trailers(trailer_order=trailer_order, copyccs=copyccs) @@ -1493,17 +1570,6 @@ class LoreSubject: self.prefixes.append(chunk) subject = re.sub(r'^\s*\[[^]]*]\s*', '', subject) self.subject = subject - # reconstitute full subject - parts = ['PATCH'] - if self.rfc: - parts.append('RFC') - if self.resend: - parts.append('RESEND') - if not self.revision_inferred: - parts.append('v%d' % self.revision) - if not self.counters_inferred: - parts.append('%d/%d' % (self.counter, self.expected)) - self.full_subject = '[%s] %s' % (' '.join(parts), subject) def __repr__(self): out = list() @@ -1631,7 +1697,8 @@ class LoreAttestorDKIM(LoreAttestor): class LoreAttestorPatatt(LoreAttestor): - def __init__(self, result: bool, identity: str, signtime: str, keysrc: str, keyalgo: str, errors: list) -> None: + def __init__(self, result: bool, identity: str, signtime: Optional[any], keysrc: str, keyalgo: str, + errors: list) -> None: super().__init__() self.mode = 'patatt' self.level = 'person' diff --git a/b4/command.py b/b4/command.py index 12c7354..5a9d6eb 100644 --- a/b4/command.py +++ b/b4/command.py @@ -118,6 +118,8 @@ def cmd(): help='Copy all Cc\'d addresses into Cc: trailers') sp_am.add_argument('--no-cover', dest='nocover', action='store_true', default=False, help='Do not save the cover letter (on by default when using -o -)') + sp_am.add_argument('--no-partial-reroll', dest='nopartialreroll', action='store_true', default=False, + help='Do not reroll partial series when detected') sp_am.set_defaults(func=cmd_am) # b4 attest @@ -129,7 +131,7 @@ def cmd(): sp_att.add_argument('-o', '--output', default=None, help='OBSOLETE: this option does nothing and will be removed') sp_att.add_argument('-m', '--mutt-filter', default=None, - help='OBSOLETE: this option does nothign and will be removed') + help='OBSOLETE: this option does nothing and will be removed') sp_att.add_argument('patchfile', nargs='*', help='Patches to attest') sp_att.set_defaults(func=cmd_attest) diff --git a/b4/diff.py b/b4/diff.py index 10f3159..38d2a9a 100644 --- a/b4/diff.py +++ b/b4/diff.py @@ -66,7 +66,7 @@ def diff_same_thread_series(cmdargs): lower = min(wantvers) else: upper = max(lmbx.series.keys()) - lower = min(lmbx.series.keys()) + lower = upper - 1 if upper == lower: logger.critical('ERROR: Could not auto-find previous revision') @@ -79,9 +79,15 @@ def diff_same_thread_series(cmdargs): if lower not in lmbx.series: return None, None + if not lmbx.series[lower].complete: + lmbx.partial_reroll(lower, sloppytrailers=False, backfill=True) + if not lmbx.series[lower].complete: lmbx.backfill(lower) + if not lmbx.series[upper].complete: + lmbx.partial_reroll(upper, sloppytrailers=False, backfill=True) + if not lmbx.series[upper].complete: lmbx.backfill(upper) diff --git a/b4/mbox.py b/b4/mbox.py index 6d4c408..f0bae35 100644 --- a/b4/mbox.py +++ b/b4/mbox.py @@ -47,7 +47,10 @@ def mbox_to_am(mboxfile, cmdargs, msgid): for key, msg in mbx.items(): lmbx.add_message(msg) - lser = lmbx.get_series(revision=wantver, sloppytrailers=cmdargs.sloppytrailers) + reroll = True + if cmdargs.nopartialreroll: + reroll = False + lser = lmbx.get_series(revision=wantver, sloppytrailers=cmdargs.sloppytrailers, reroll=reroll) if lser is None and wantver is None: logger.critical('No patches found.') return @@ -156,6 +159,11 @@ def mbox_to_am(mboxfile, cmdargs, msgid): logger.info('Prepared a fake commit range for 3-way merge (%.12s..%.12s)', rstart, rend) logger.critical('---') + if lser.partial_reroll: + logger.critical('WARNING: v%s is a partial reroll from previous revisions', lser.revision) + logger.critical(' Please carefully review the resulting series to ensure correctness') + logger.critical(' Pass --no-partial-reroll to disable') + logger.critical('---') if not lser.complete and not cmdargs.cherrypick: logger.critical('WARNING: Thread incomplete!') -- cgit v1.2.3