[PATCH] HTTP: factor out server name into a constant

[Maxim Dounin]

Hello!

On Sat, Jun 06, 2020 at 08:01:57PM -0500, Varun Varada wrote:

> On Sat, 6 Jun 2020 at 18:18, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >
> > On Sat, Jun 06, 2020 at 04:08:40PM -0500, Varun Varada wrote:
> >
> > > # HG changeset patch
> > > # User Varun Varada <varuncvarada at gmail.com>
> > > # Date 1591475111 18000
> > > #      Sat Jun 06 15:25:11 2020 -0500
> > > # Node ID f37aa29453e006bdc37cbe7d9f65eec0de27b731
> > > # Parent  699f6e55bbb4672632e7def5c65b1dbae2960380
> > > HTTP: factor out server name into a constant
> > >
> > > This commit factors out the name of the server ("nginx") into a
> > > constant to make the code DRY and so that modifying the server
> > > name, if necessary, can be done in one place.
> >
> > Thank you for the patch, but no.  While this change may help
> > people "modifying the server name", such a change would have a
> > negative impact on nginx itself.
> 
> Modifying the server name is a secondary concern. Currently, the name
> of the server is hard-coded in multiple places which is not a best
> practice.

Thank you for your opinion.

In your patch, the name of the server, nginx, is hardcoded in 
eleven additional places compared to the original code.  So, with 
your patch we'll have eleven additional places to change if we'll 
ever decide to rename nginx to something else.

The fact that the string to be returned to HTTP clients can be 
changed in a single place is irrelevant as it is not expected to 
be changed separately from changing the name of the server.

Not to mention the patch breaks pre-encoding of the name with 
HPACK in HTTP/2, and hence implies run-time costs.

Overall, this looks like clearly negative change.  And this is 
what was written above in the short form: "negative impact on 
nginx itself".

> > Consider looking at this ticket instead:
> >
> > https://trac.nginx.org/nginx/ticket/1644
> 
> That seems to have to do with a behavioural issue, which is unrelated
> to making the code DRY.

Sure, this is a behavioural issue.  And also this is the only 
understandable reason for the patch you've suggested, since the 
change in question clearly makes the code worse.

If you indeed think that the patch "makes the code DRY", and 
therefore it is a good thing to do - you may want to reconsider 
how to apply the DRY principle, you are doing it wrong (hint: in 
many cases duplicating things is beneficial).

If you are instead lying to yourself trying to pretend that 
"modifying the server name is a secondary concern", and want 
instead make it easier to modify the string returned to HTTP 
clients, then please refer to the ticket linked above.  We are not 
going to accept patches making such modifications easier.  Instead 
of trying to submit such patches, consider making it clear - for 
yourself in the first place - that such modifications should not 
be done.

-- 
Maxim Dounin
http://mdounin.ru/