Re: [PATCH v2 10/10] cat-file: use writev(2) if available

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 27, 2024, 15:43 UTC
Message-ID: <xmqqwmk2as3w.fsf@gitster.g>
In-Reply-To: <xmqq1q2acyjo.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 36 quoted lines

>> +static void batch_writev(struct batch_options *opt, struct expand_data *data,
>> +			const struct strbuf *hdr, size_t size)
>> +{
>> ...
>> +}
>
> ... the above made me read it twice, wondering "where does
> iov[1].iov_base comes from???"  The location of the git_iovec
> structure in the expand_data forces this rather unnatural calling
> convention where the iovec is passed by address (as part of the
> expand_data structure), with only one of six slots filled, and the
> other five slots are filled by this function from the parameters
> passed to it.
>
> I wonder if we can rework the data structure to
>
>  - Not embed git_iovec iov[] in expand_data;
>
>  - Keep "void *content" instead there;
>
>  - Define an on-stack "struct git_iovec iov[3]" local to this function;
>
>  - Pass "void *content" from the caller to this function;
>
>  - Populate iov[] fully from hdr->{buf,len}, content, size, and
>    opt->output_delim and consume it in this function by either
>    calling fwritev_or_die() or writev_or_die().
>
> That way, the caller does not have to use data->iov[1].iov_base in
> place of data->content, which is the source of "Huh?  Why is the 2nd
> element of the 3-element array so special?" puzzlement readers would
> feel while reading the caller---after all, the fact that we are
> using writev with three chunks is an implementation detail that the
> caller does not have to know to correctly use this helper function.
>
> Or am I missing something?

Additional thought.  Perhaps we can introduce

    static void batch_write(struct batch_options *opt,
			    const void *data, ...);

that is a vararg function that takes <data, len> pairs repeated at the end, with data==NULL as sentinel. It may technically need to be called batch_writel(), but that is a backward compatible interface for existing batch_write() callers.

Then the use of writev() can be encapsulated inside the updated batch_write() function. If you get only a single <data, len> pair, you would do a single write_or_die() or fwrite_or_die(). Otherwise you'd do the writev() thing, and the function can stay oblivious to the meaning of what it is writing out. There is no need for the function to know that the payload is "header followed by body followed by delimiter byte", as that is what the callers express at the call sites of the function.

Hmm?