Re: [PATCH 1/5] xdiff: support external hunks via xpparam_t
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 22, 2026, 05:29 UTC
- Message-ID
- <xmqq33zkui4q.fsf@gitster.g>
- In-Reply-To
- <8c0ea0bc0742651e634db7a3002e8cbe1240acf9.1779415884.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 24 quoted lines
> +/*
> + * Populate the changed[] arrays from externally supplied hunks,
> + * bypassing the diff algorithm. Validates that hunks are in order,
> + * non-overlapping, and within bounds.
> + *
> + * Returns 0 on success, -1 on validation failure.
> + */
> +static int xdl_populate_hunks_from_external(xdfenv_t *xe,
> + const struct xdl_hunk *hunks,
> + size_t nr_hunks)
> +{
> + size_t i;
> + long j, prev_old_end = 0, prev_new_end = 0;
> + long total_old = 0, total_new = 0;
> +
> + /*
> + * 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.
Show 23 quoted lines
> int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
> xdchange_t *xscr;
> xdfenv_t xe;
> emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
>
> - if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
> -
> - return -1;
> + if (xpp->external_hunks) {
> + if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
> + return -1;
> + if (xdl_populate_hunks_from_external(&xe,
> + xpp->external_hunks,
> + xpp->external_hunks_nr) < 0) {
> + /*
> + * Invalid external hunks; fall back to the
> + * builtin diff algorithm. Re-runs
> + * xdl_prepare_env() via xdl_do_diff().
> + */
> + xdl_free_env(&xe);
> + if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> + return -1;If the external tool keeps sending bogus hunks, silently falling back to what we would have done if there weren't any external stuff may be necessary to pleasantly keep using Git, but two and a half short comments here.
(1) "What we would have done" is exactly the same as what appears
in the corresponding "else" block. Can we make sure that we do
not have to keep updating both copies in the future with some
code rearrangement? (2) The writer of the external tool may want to see some trace of
warning under certain flags when a failure of the tool forces
the receiving end to fallback. (3) If the tool throws too many broken replies, perhaps we want to
disable it automatically?Show 9 quoted lines
> + }
> + } else {
> + if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> + return -1;
> }
> +
> if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
> xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
> xdl_build_script(&xe, &xscr) < 0) {