From: Michael Montalbo Date: Sun, 24 May 2026 18:01:29 GMT Subject: Re: [PATCH 1/5] xdiff: support external hunks via xpparam_t Message-ID: In-Reply-To: On Sun, May 24, 2026 at 1:50 AM Junio C Hamano wrote: > > Michael Montalbo writes: > > >> > + * Clear changed[] arrays. xdl_prepare_env() may have dirtied > >> > + * them via xdl_cleanup_records(). The allocation is nrec + 2 > >> > + * elements; changed points one past the start (see xprepare.c). > >> > + */ > >> > + memset(xe->xdf1.changed - 1, 0, > >> > + (xe->xdf1.nrec + 2) * sizeof(bool)); > >> > + memset(xe->xdf2.changed - 1, 0, > >> > + (xe->xdf2.nrec + 2) * sizeof(bool)); > >> > >> This, especially the starting offset of -1, looks horrible. The > >> internal layout of xdfenv_t might happen to match the way the above > >> code expects, which is how xdl_prepare_ctx() may have give you, but > >> it somehow feels brittle. I guess the assumption that changed[] > >> does not point at the beginning of the allocated area (e.g., it is a > >> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that > >> it cannot be helped. Sigh. > >> > > > > Agreed it is ugly. I wanted to make sure the entire changed[] including > > sentinels were clear as a defensive measure for downstream callers > > (xdl_change_compact). I agree this results in something that is ugly > > and brittle, but in the end I thought it was superior to relying on the > > fact that upstream zeroes the entire changed[] array. Maybe if the > > comment was more explicit about why this is happening it would be > > helpful? > > Perhaps make these memset() into calls to a helper function that is > defined in xdiff/xprepare.c with a descriptive name and placed near > where xdl_prepare_ctx() is. That way, the patch in question does > not even have to expose the strangeness of changed[] (i.e., it has 2 > more elements than it would normally contain to make the memory > region for changed[-1] and changed[N] valid, and freeing it requires > free(changed-1)) to the code path. It only needs to say "Hey, I am > clearing changed[] arrays because of XXX" without having to say "by > the way, the memory layout of changed[] is strange this way", the > latter of which is not exactly of interest for readers of this code. > > > /* > > * Clear changed[] arrays including sentinels. > > * xdl_prepare_env() may have dirtied them via > > * xdl_cleanup_records(), and xdl_change_compact() reads > > * the sentinel at changed[-1] during backward scans. > > */ > > And this belongs in xdiff/xprepare.c near that new helper function. That sounds a lot nicer. Will update.