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@mdounin.ru> wrote:
> >
> > On Sat, Jun 06, 2020 at 04:08:40PM -0500, Varun Varada wrote:
> >
> > > # HG changeset patch
> > > # User Varun Varada <varuncvarada@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel