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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Feb 12, 2025 at 11:24:55AM -0600, Justin Tobler wrote:
> On 25/02/12 10:23AM, Patrick Steinhardt wrote:
> > 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.

Okay. This is context that should probably be part of the commit
message, as it is quite an important detail to understand the
implementation.

Patrick