Re: [PATCH] diff-lib: ignore all outsider if --relative asked

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2021, 16:55 UTC
Message-ID: <xmqqwnoh2x2i.fsf@gitster.g>
In-Reply-To: <bc7eda4ed8d52072b929a4af6e4e4ed7478ef9d6.1629361733.git.congdanhqx@gmail.com>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Show 15 quoted lines

> For diff family commands, we can tell them to exclude changes outside
> of some directories if --relative is requested.
>
> In diff_unmerge(), NULL will be returned if the requested path is
> outside of the interesting directories, thus we'll run into NULL
> pointer dereference in run_diff_files when trying to dereference
> its return value.
>
> We can simply check for NULL there before dereferencing said
> return value.  However, we can do better by not running diff
> on those unintesting entries.  Let's do that instead.
>
> Reported-by: Thomas De Zeeuw <thomas@slight.dev>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---

Nicely done.

If we look at cd676a51 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) where the "--relative" feature was introduced a bit more carefully, we notice that it wanted to implement "anything outside the .prefix gets discarded" at diff_addremove(), diff_change(), and diff_unmerge() level, instead of the side that enumerates the paths and calls these helpers, and that way, the "--relative" feature would consistently work across diff-files, diff-tree, and diff-index, as they all share these three helpers.

But filtering upfront before the codepath even has to decide if it needs to call diff_addremove() or diff_change(), like this patch does, makes sense, especially in the context of diff-files where the enumeration of paths is just to walk a single flat array that is the in-core index.

The proposed log message needs a bit more work, though. It would be an 80% OK explanation if the "check diff_unmerge()'s return value" approach was sufficient to correct bugs and we took the approach, but that is not the case. As you found out, it is not sufficient, and it is not the approach you took. The only part in the proposed log that explains the approach that was actually taken was "we can do better by ...".

Until/unless we do similar "filter with diffopt.prefix upfront" in diff-index and diff-tree codepaths, we unfortunately cannot lose the filter added to diff_addremove() and diff_change(), but I think this is a good first step towards such a longer-term clean-up.

Thanks.