Re: [PATCH v3 1/3] remote-curl: use strlen() instead of magic numbers

Jeff King [off-list ref] writes:

On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote:

quoted

When we are dealing with a packet-line length header, we use the magic
literal `4`, representing the length of "0000" and "0001", the packet
line length headers. Use `strlen("000x")` so that we do not have to use
the magic literal.

I'm not a huge fan of using strlen() for this, because it's _still_
magical (you cannot change "0000" in one place without changing it in
another"). And while it helps with understanding that "4" matches the
length of that string, IMHO it's harder to read because now I have to
make sure that those much longer strings all match up.

Yup.  There are two instances of recurring pattern with two memcpy,
where three copies of the same string must appear.  Unless the whole
thing is abstracted away so that these two instances are calls to
a macro/function that takes a single "0000" (and "0001"), I do not
think it is an improvement.

This refactoring also implies to me that if you changed all of "0000" on
one line you'd be fine, but that's emphatically not true. The magic
number "4" is used to size the buffer earlier in the function, and would
have to match (and of course since this is a network protocol, it's not
like you could even change those in isolation).

Thanks.  That's even more important point.

So I dunno. I kind of think the raw "4" is the most readable. It's quite
obvious to me in the context of a memcpy() what's going on. I don't mind
memcpy_literal() or similar that hides the repetition, but I think it's
hard to do here because of the arithmetic on the destination.

-Peff