Re: [PATCH v3 04/10] xdiff: delete xdl_get_rec() in xemit

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2025, 17:48 UTC
Message-ID: <xmqq8qi9xbf1.fsf@gitster.g>
In-Reply-To: <ddfee67e06bb56cfa8f11187dc27135bfce63647.1758294992.git.gitgitgadget@gmail.com>

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> This function aliases the fields of xrecord_t, which makes it harder
> to track the usages of those fields. Delete it.

Can we phrase it in a way that is a bit more friendly to casual readers? It is hard to tell if the function is serving any useful purpose from the above. If it is doing something useful, but is hard to read the surrounding code, that wouldn't be a good reason to remove it.

It seems that this patch does what people often call "inlining a function call". I am not sure your comment about aliasing or if that is why the code is harder than necessary to read.

Show 6 quoted lines

> -static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb) {
> -	long size, psize = strlen(pre);
> -	char const *rec;
> -
> -	size = xdl_get_rec(xdf, ri, &rec);
> -	if (xdl_emit_diffrec(rec, size, pre, psize, ecb) < 0) {

So we used to get "rec" and "size" separately because xdl_get_rec() made xrecord_t inaccessible to its callers. To call a helper function that takes a <ptr, len> pair, xdl_get_rec() is the way to grab that <ptr, len> pair out of the record index.

Show 7 quoted lines

> +static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb)
> +{
> +	xrecord_t *rec = xdf->recs[ri];
>  
> +	if (xdl_emit_diffrec(rec->ptr, rec->size, pre, strlen(pre), ecb) < 0) {
>  		return -1;
>  	}

But by directly peeking into the xdf->recs[] array, we do not have to. Each element of the array is the <ptr, len> pair we want.

The updated code is certainly easier to read. That applies to all the other callers touched by this patch.

Thanks.