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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2020, 19:39 UTC
Message-ID: <xmqqtuz1bmgz.fsf@gitster.c.googlers.com>
In-Reply-To: <20200623185458.GD1444619@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

> On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote:
>
>> 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.

Show 5 quoted lines

> 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.

Show 6 quoted lines

> 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