Re: [PATCH v5 1/4] diff: return diff_filepair from diff queue helpers

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2025, 16:17 UTC
Message-ID: <xmqqpliyxf5u.fsf@gitster.g>
In-Reply-To: <20250228213346.1335224-2-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 9 quoted lines

> The `diff_addremove()` and `diff_change()` functions set up and queue
> diffs, but do not return the `diff_filepair` added to the queue. In a
> subsequent commit, modifications to `diff_filepair` need to occur in
> certain cases after being queued.
>
> Since the existing `diff_addremove()` and `diff_change()` are also used
> for callbacks in `diff_options` as types `add_remove_fn_t` and
> `change_fn_t`, modifying the existing function signatures requires
> further changes.

Sensible. The patch presented below looks a sane and safe no-op for the existing code paths, which is what we want to see in a preliminary refactoring step like this one.

> The diff options for pruning use `file_add_remove()`
> and `file_change()` where file pairs do not even get queued. Thus,
> separate functions are implemented instead.

This looked a bit confusing, but it is an explanation for the reason why we simply do not change the function signature of the callback members of diff_options structure. These addremove/change callbacks are designed to be a general way to allow applications to react to discovered changes to paths, and I agree that it makes sense for them to be usable to perform something that has nothing to do with the diff_queue structure.

> Split out the queuing operations into `diff_queue_addremove()` and
> `diff_queue_change()` which also return a handle to the queued
> `diff_filepair`. Both `diff_addremove()` and `diff_change()` are
> reimplemented as thin wrappers around the new functions.

Nice.