Re: constants for tar header offsets

On Wed Apr 19, 2023 at 8:09 AM CDT, Robert Haas wrote:
> On Tue, Apr 18, 2023 at 12:56 PM Dagfinn Ilmari Mannsåker
>  wrote:
> > It still has magic numbers for the sizes of the fields, should those
> > also be named constants?
>
> I thought about that. It's arguable, but personally, I don't think
> it's worth it. If the concern is greppability, having constants for
> the offsets is good enough for that. If the concern is making it
> error-free, I think we'd be well-advised to consider bigger redesigns
> of the API. For example, we could have a function
> read_number_from_tar_header(char *pointer_to_the_start_of_the_block,
> enum which_field) and then that function could encapsulate the
> knowledge of which tar numbers are 8 bytes and which are 12 bytes.
> Writing read_number_from_tar_header(h, TAR_FIELD_CHECKSUM) seems
> potentially less error-prone than
> read_tar_number(&h[TAR_OFFSET_CHECKSUM], 8). On the other hand,
> changing the latter to read_tar_number(&h[TAR_OFFSET_CHECKSUM],
> TAR_LENGTH_CHECKSUM) seems longer but not necessarily cleaner. So I
> felt it didn't make sense.
>
> Just to be clear, I don't have a full vision for what a replacement
> API ought to look like, and I'm not sure that figuring that out is
> something that has to be done right this minute. I proposed this patch
> not because it's perfect, but because it's simple. We can think of
> doing more in the future if someone wants to devote the effort, and
> that person might even be me, but right now it's not.

A new API design would be great, but for right now v2 is good enough and 
should be committed. It is much easier to read the code with this patch 
applied.

Marking as "Ready for Committer" since we all seem to agree that this is 
better than what exists.

-- 
Tristan Partin
Neon (https://neon.tech)