| Message ID | 20161123070131.28065-1-andrew.donnellan@au1.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
On 23/11/16 18:01, Andrew Donnellan wrote: > When generating an mbox for a patch with tags in the original commit > message, e.g.: > > Example patch > > This patch is awesome! > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Acked-by: Russell Currey <ruscur@russell.cc> > > the tags from the original email are duplicated: > > Example patch > > This patch is awesome! > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Acked-by: Russell Currey <ruscur@russell.cc> > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Acked-by: Russell Currey <ruscur@russell.cc> > > It appears that during the refactoring in ef56359fb776 ("models: Merge > patch and first comment"), we added a call to patch.patch_responses() to > extract the tags from the initial patch email, which we then append to the > patch email body... which already has the tags in it. > > Remove the unnecessary append of patch.patch_responses when generating an > mbox. > > Fixes: ef56359fb776 ("models: Merge patch and first comment") > Reported-by: Russell Currey <ruscur@russell.cc> > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > --- > > Stephen, does this look correct? Would it be helpful to add a test that compares full mbox output to catch stuff like this?
> Would it be helpful to add a test that compares full mbox output to > catch stuff like this? IMO, the answer to "would it be helpful to have a test for this thing we just discovered was broken?" is always "yes". Anyway, I'm happy if that's a follow-up patch. Reviewed-by: Daniel Axtens <dja@axtens.net> Regards, Daniel > > -- > Andrew Donnellan OzLabs, ADL Canberra > andrew.donnellan@au1.ibm.com IBM Australia Limited > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Thu, 2016-11-24 at 11:28 +1100, Daniel Axtens wrote: > > > > Would it be helpful to add a test that compares full mbox output > > to > > catch stuff like this? > > IMO, the answer to "would it be helpful to have a test for this thing > we > just discovered was broken?" is always "yes". +1 - a test would be appreciated. > Anyway, I'm happy if that's a follow-up patch. > > Reviewed-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Stephen Finucane <stephen@that.guru> ...and applied.
diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index a29da83..58fb29f 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -368,8 +368,6 @@ def patch_to_mbox(patch): postscript = '' # TODO(stephenfin): Make this use the tags infrastructure - body += patch.patch_responses - for comment in Comment.objects.filter(submission=patch): body += comment.patch_responses
When generating an mbox for a patch with tags in the original commit message, e.g.: Example patch This patch is awesome! Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> Acked-by: Russell Currey <ruscur@russell.cc> the tags from the original email are duplicated: Example patch This patch is awesome! Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> Acked-by: Russell Currey <ruscur@russell.cc> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> Acked-by: Russell Currey <ruscur@russell.cc> It appears that during the refactoring in ef56359fb776 ("models: Merge patch and first comment"), we added a call to patch.patch_responses() to extract the tags from the initial patch email, which we then append to the patch email body... which already has the tags in it. Remove the unnecessary append of patch.patch_responses when generating an mbox. Fixes: ef56359fb776 ("models: Merge patch and first comment") Reported-by: Russell Currey <ruscur@russell.cc> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> --- Stephen, does this look correct? --- patchwork/views/__init__.py | 2 -- 1 file changed, 2 deletions(-)