From: Junio C Hamano Date: Mon, 03 Mar 2025 16:17:01 GMT Subject: Re: [PATCH v5 1/4] diff: return diff_filepair from diff queue helpers Message-ID: In-Reply-To: <20250228213346.1335224-2-jltobler@gmail.com> Justin Tobler writes: > 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.