Comment 10 for bug 1394403

Robie Basak (racb) wrote :

Thanks Wesley, this looks good.

Some minor changes please:

1) Please could you add an Origin: header to the debdiff? Something like "Origin: upstream," or "Origin: backport," depending on whether you had to tweak the diff or not. Sorry I didn't mention this on IRC before - I saw the Closes/LP thing on a quick glance but I didn't review properly.

2) Please could you note what's going on with the new DirectoryCheckHandler directive in the debian/changelog entry? In particular, I think it should answer: should users expect any behaviour changes when updating and not changing anything; and what users need to do to fix the bug after the update is applied (set DirectoryCheckHandler to something?)

3) Given that the directive is being added, it should probably be noted in the Regression Potential section, along with stating whether default behaviour is being changed or not, and we should probably add a test case to the Test Case section to make sure that the path that shouldn't change is also exercised to ensure that it indeed hasn't changed.

I think I'm asking for 2 and 3 really because I don't feel that I yet have clarity on exactly which way round the new directive works, and what expectations are with regard to behaviour changes on the SRU update (rather than against 2.2). I understand that parity against 2.2 is needed to fix the bug, but from an SRU and regression perspective it's behaviour changes on the SRU update itself that I'm bothered about. Essentially, it comes down to: how are we ensuring that no user will scream at us for breaking behaviour in pushing this SRU?

I'm sorry that this is a bit more complicated that I originally expected when I asked you to look at this because of the behaviour issue.

With these changes, assuming it builds and you've tested it then I'm happy to sponsor an upload. Thanks!

Log from IRC:

11:30 <rbasak> mdeslaur: around? I'm looking at sponsoring bug 1394403 - as you're looked at it before I'd like your opinion please.

11:30 <ubottu> bug 1394403 in apache2 (Ubuntu Trusty) "RewriteRule of "^$" is broken" [Medium,Confirmed]

11:31 <rbasak> when I asked magicalChicken to look at it I didn't realise the upstream fix would add a configuration directive. But it looks like it's safe as it defaults to the same behaviour. Had you considered this already? Does it also look reasonable to you?

11:31 <rbasak> I also think we should include the documentation update in our backport - better than not having it in the SRU IMHO.

12:10 <mdeslaur> rbasak: I'm ok with the new config option...I've added options before to packages as security updates, so it's not like we haven't done it before. The option will change the behaviour though, but cases where it will break something are unlikely

12:10 <mdeslaur> rbasak: for the documentation, meh...if it were man pages, I'd push for it...but the static web documentation, meh

12:10 <mdeslaur> rbasak: especially since there are localized versions of the documentation and we'd only be updating the english version

12:11 <mdeslaur> rbasak: the only thing is perhaps add what the option is and how the default has changed to the changelog

12:16 <rbasak> mdeslaur: OK. Thanks!