Re: [PATCH v3 05/10] bulk-checkin: extract abstract `bulk_checkin_source`

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> A future commit will want to implement a very similar routine as in
> `stream_blob_to_pack()` with two notable changes:
>
>   - Instead of streaming just OBJ_BLOBs, this new function may want to
>     stream objects of arbitrary type.
>
>   - Instead of streaming the object's contents from an open
>     file-descriptor, this new function may want to "stream" its contents
>     from memory.
>
> To avoid duplicating a significant chunk of code between the existing
> `stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This
> concept currently is a thin layer of `lseek()` and `read_in_full()`, but
> will grow to understand how to perform analogous operations when writing
> out an object's contents from memory.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  bulk-checkin.c | 61 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 8 deletions(-)

> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index f4914fb6d1..fc1d902018 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -140,8 +140,41 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
>  	return 0;
>  }
>  
> +struct bulk_checkin_source {
> +	enum { SOURCE_FILE } type;
> +
> +	/* SOURCE_FILE fields */
> +	int fd;
> +
> +	/* common fields */
> +	size_t size;
> +	const char *path;
> +};

Looks OK, even though I expected to see a bit more involved object
orientation with something like

	struct bulk_checkin_source {
		off_t (*read)(struct bulk_checkin_source *, void *, size_t);
		off_t (*seek)(struct bulk_checkin_source *, off_t);
		union {
			struct {
				int fd;
				size_t size;
				const char *path;
			} from_fd;
			struct {
				...
			} incore;
		} data;
	};

As there will only be two subclasses of this thing, it may not
matter all that much right now, but it would be much nicer as your
methods do not have to care about "switch (enum) { case FILE: ... }".