Re: [PATCH v2 1/3] diff: return diff_filepair from diff queue helpers

From: Justin Tobler <jltobler@gmail.com>
Date: Feb 12, 2025, 17:24 UTC
Message-ID: <3vqcdil4qdam2pcb3mjruuda2kgph3arzhwswnlugtqykmc6zm@7jiwkyqty5lj>
In-Reply-To: <Z6xomFk_Sb18UMFo@pks.im>

On 25/02/12 10:23AM, Patrick Steinhardt wrote:

Show 14 quoted lines

> On Tue, Feb 11, 2025 at 10:18:23PM -0600, Justin Tobler wrote:
> > The `diff_addremove()` and `diff_change()` functions setup and queue
> > diffs, but do not return the `diff_filepair` added to the queue. In a
> > subsequent commit, modifications to `diff_filepair` need to take place
> > in certain cases after being queued.
> > 
> > Split out the queuing operations into `diff_filepair_addremove()` and
> > `diff_filepair_change()` which also return a handle to the queued
> > `diff_filepair`.
> 
> One of the things that puzzled me a bit is that we keep the old-style
> functions, where the only difference is the return value. Wouldn't it
> make more sense to instead adapt these existing functions to reduce the
> amount of duplication?

This is what I considered doing initially. I noticed though that both `diff_addremove()` and `diff_change()` are stored as callbacks in `diff_options` as types `add_remove_fn_t` and `change_fn_t`. The diff options configured for pruning use `file_add_remove()` and `file_change()` instead. Returning `diff_filepair` doesn't seems to make much sense in the context of `file_add_remove()` and `file_change()` as no filepairs ever get queued, so I opted to factor out the logic into separate functions instead of adapting the function signatures for all.

This may not be the best option, so I can also change it if that is best.

Show 6 quoted lines

> At the same time, while we're already at it, do we maybe also want to
> adapt the functions so that they get the `diff_queue` as input instead
> of relying on the global queue? That would make them more generally
> useful and be a step into the right direction regarding libification. If
> so, it would indeed make sense to also rename the function into e.g.
> `diff_queue_addremove()`.

Thanks for the suggestion. I'll adapt the next version accordingly.

-Justin