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

[Junio C Hamano <gitster@pobox.com>]

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

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