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
