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. > 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
