On Mon, 2017-06-26 at 21:01 +1000, Daniel Axtens wrote: > Hi Stephen, > > > Currently, once we've found a series for a new submission, we check to > > ensure that the version of that series matches that of the submission, > > and we discard the series if not. The original intention for this was > > given in commit 'c21b30525', where this was added: > > > > There are some things you probably shouldn't do on public mailing > > lists, but which people do anyway. > > > > - [PATCH 1/2] test: Add some lorem ipsum > > - [PATCH 2/2] test: Convert to Markdown > > - [PATCH v2 1/2] test: Add some lorem ipsum > > - [PATCH v2 2/2] test: Convert to Markdown > > > > We should correctly parse these... > > > > This is unnecessary for two reasons: > > > > - If the series is using proper references, then the mechanism that > > we use to prevent this issue with unversioned follow ups (also added > > in that patch) will work here: namely, we don't add submissions if an > > submission with a given number already exists in that series. > > - If the series is not using references, we already have a check for > > version. > > > > Seeing as this check is actually causing issues for legitimate typos, we > > should just remove this check. Do so, adding a check to ensure we don't > > regress. > > I'd really really really prefer people used git-send-email and/or > whatever the mercurial equivalent is. The tools aren't *that* difficult > to use correctly. But having said that, I'm not necessarily opposed to > this patch. Not super keen, but not super opposed.
In general, I would agree. However, it is surprisingly easy to make this mistake yourself. If I weren't aware of the 'git-send-email --reroll-count' option, I would probably default to encoding the version in the subject line of the cover letter. We actually assume that some people will do this in our version check [1]. Coupled with the fact that this check doesn't make any of our existing cases less robust (see above), I think this is a prudent move and a real world demonstration of the Robustness Principle [2]. FWIW, the Robustness Principle was the eventual reason for not merging patches to optionally restrict the parser to 'git-send-email'-formatted patches. > 1 comment below. > > Regards, > Daniel > > > > > Signed-off-by: Stephen Finucane <[email protected]> > > Closes-bug: #105 > > --- > > patchwork/parser.py | 1 - > > .../tests/series/base-different-versions.mbox | 908 > > +++++++++++++++++++++ > > patchwork/tests/test_series.py | 18 + > > 3 files changed, 926 insertions(+), 1 deletion(-) > > create mode 100644 patchwork/tests/series/base-different-versions.mbox > > > > diff --git a/patchwork/parser.py b/patchwork/parser.py > > index 794a5ea..c43fe76 100644 > > --- a/patchwork/parser.py > > +++ b/patchwork/parser.py > > @@ -911,7 +911,6 @@ def parse_mail(mail, list_id=None): > > # * the version doesn't match > > You'll need to remove this comment line too. Good call. I can do this when I apply the patch. Stephen [1] https://github.com/getpatchwork/patchwork/blob/v2.0.0-rc4/patchwork/parser. py#L407-L409 [2] https://en.wikipedia.org/wiki/Robustness_principle _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
