Re: [PATCH 02/15] chunk-format: add API for writing table of contents

[Taylor Blau <me@ttaylorr.com>]

On Thu, Dec 03, 2020 at 04:16:41PM +0000, Derrick Stolee via GitGitGadget wrote:
> @@ -0,0 +1,26 @@
> +#include "git-compat-util.h"
> +#include "chunk-format.h"
> +#include "csum-file.h"
> +#define CHUNK_LOOKUP_WIDTH 12
> +
> +void write_table_of_contents(struct hashfile *f,
> +			     uint64_t cur_offset,

Hmmph. Could we not use hashfile_total() here instead? I wonder how much
the callers would be able to be cleaned up without having to keep track
of the offset themselves.

Is this to indicate that we might call hashfd() on a file that already
has data in it at an offset that doesn't cause us to overwrite that
data? I doubt it (since both callers currently write all of their data
into a hashfile), but if so that should be documented, too.

(In either case, off_t is a more appropriate type than uint64_t here.)

> @@ -0,0 +1,36 @@
> +#ifndef CHUNK_FORMAT_H
> +#define CHUNK_FORMAT_H
> +
> +#include "git-compat-util.h"
> +
> +struct hashfile;
> +
> +typedef int (*chunk_write_fn)(struct hashfile *f,
> +			      void *data);
> +
> +/*
> + * When writing a chunk-based file format, collect the chunks in
> + * an array of chunk_info structs. The size stores the _expected_

I'm not clear on what "_expected_" means here. Does that mean that the
actual data written may be over- or under-sized? If not, does violating
the expectation cause an error?

I realize that I'm being pedantic, but documenting this would be useful
to callers.

> + * amount of data that will be written by write_fn.
> + */
> +struct chunk_info {
> +	uint32_t id;
> +	uint64_t size;
> +	chunk_write_fn write_fn;
> +};
> +
> +/*
> + * Write the chunk data into the supplied hashfile.
> + *
> + * * 'cur_offset' indicates the number of bytes written to the hashfile
> + *   before the table of contents starts.
> + *
> + * * 'nr' is the number of chunks with non-zero IDs, so 'nr + 1'
> + *   chunks are written in total.
> + */
> +void write_table_of_contents(struct hashfile *f,
> +			     uint64_t cur_offset,

Same goes here about cur_offset.

Thanks,
Taylor