All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
To: git@vger.kernel.org
Cc: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Thomas De Zeeuw" <thomas@slight.dev>,
	"Carlo Arenas" <carenas@gmail.com>
Subject: [PATCH v3] diff-lib: ignore all outsider if --relative asked
Date: Sun, 22 Aug 2021 15:49:08 +0700	[thread overview]
Message-ID: <a140b292f94de9f4d96f79f3817ed7d2d609c7ba.1629621310.git.congdanhqx@gmail.com> (raw)
In-Reply-To: <0d73c7181969d2916d71d7aeb6d788324a0db68b.1629514355.git.congdanhqx@gmail.com>

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.

Checking for return value of diff_unmerge before dereferencing
is not sufficient, though. Since, diff engine will try to work on such
pathspec later.

Let's not run diff on those unintesting entries, instead.
As a side effect, by skipping like that, we can save some CPU cycles.

Reported-by: Thomas De Zeeuw <thomas@slight.dev>
Tested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Sorry for the noise, I need to send v3, because:

Change in v3 from v2:
 Fix the conflict marker in failure test. Originally, I wrote the test with:
 "git merge sub1" not "git merge br1"

Range-diff against v2:
1:  0d73c71819 ! 1:  a140b292f9 diff-lib: ignore all outsider if --relative asked
    @@ t/t4045-diff-relative.sh: check_diff_relative_option subdir file2 true --no-rela
     +	 +sub3
     +	++=======
     +	+ sub1
    -+	++>>>>>>> sub1
    ++	++>>>>>>> br1
     +	EOF
     +	git -C subdir diff --relative >out &&
     +	test_cmp expected out

Range-diff of v2 against v1:
1:  57a9edc3af ! 1:  0d73c71819 diff-lib: ignore all outsider if --relative asked
    @@ Commit message
         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.
    +    Checking for return value of diff_unmerge before dereferencing
    +    is not sufficient, though. Since, diff engine will try to work on such
    +    pathspec later.
    +
    +    Let's not run diff on those unintesting entries, instead.
    +    As a side effect, by skipping like that, we can save some CPU cycles.
     
         Reported-by: Thomas De Zeeuw <thomas@slight.dev>
    +    Tested-by: Carlo Arenas <carenas@gmail.com>
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
 diff-lib.c               |  4 +++
 t/t4045-diff-relative.sh | 53 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index f9eadc4fc1..ca085a03ef 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -117,6 +117,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))
 			continue;
 
+		if (revs->diffopt.prefix &&
+		    strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length))
+			continue;
+
 		if (ce_stage(ce)) {
 			struct combine_diff_path *dpath;
 			struct diff_filepair *pair;
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 61ba5f707f..fab351b48a 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -162,4 +162,57 @@ check_diff_relative_option subdir file2 true --no-relative --relative
 check_diff_relative_option . file2 false --no-relative --relative=subdir
 check_diff_relative_option . file2 true --no-relative --relative=subdir
 
+test_expect_success 'setup diff --relative unmerged' '
+	test_commit zero file0 &&
+	test_commit base subdir/file0 &&
+	git switch -c br1 &&
+	test_commit one file0 &&
+	test_commit sub1 subdir/file0 &&
+	git switch -c br2 base &&
+	test_commit two file0 &&
+	git switch -c br3 &&
+	test_commit sub3 subdir/file0
+'
+
+test_expect_success 'diff --relative without change in subdir' '
+	git switch br2 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge one &&
+	git -C subdir diff --relative >out &&
+	test_must_be_empty out &&
+	git -C subdir diff --relative --name-only >out &&
+	test_must_be_empty out
+'
+
+test_expect_success 'diff --relative --name-only with change in subdir' '
+	git switch br3 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge sub1 &&
+	test_write_lines file0 file0 >expected &&
+	git -C subdir diff --relative --name-only >out &&
+	test_cmp expected out
+'
+
+test_expect_failure 'diff --relative with change in subdir' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	cat >expected <<-EOF &&
+	diff --cc file0
+	index $br3_blob,$br1_blob..0000000
+	--- a/file0
+	+++ b/file0
+	@@@ -1,1 -1,1 +1,5 @@@
+	++<<<<<<< HEAD
+	 +sub3
+	++=======
+	+ sub1
+	++>>>>>>> br1
+	EOF
+	git -C subdir diff --relative >out &&
+	test_cmp expected out
+'
+
 test_done
-- 
2.33.0.254.g68ee769121


      parent reply	other threads:[~2021-08-22  8:49 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-21  4:03 [PATCH v2] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
2021-08-18  8:42 ` Bug: Segmentation fault in git diff Thomas De Zeeuw
2021-08-18 10:44   ` Đoàn Trần Công Danh
2021-08-18 12:52     ` Đoàn Trần Công Danh
2021-08-18 20:30       ` Junio C Hamano
2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
2021-08-19  9:02     ` Carlo Arenas
2021-08-19 16:58       ` Junio C Hamano
2021-08-19 16:55     ` Junio C Hamano
2021-08-22  8:49     ` Đoàn Trần Công Danh [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a140b292f94de9f4d96f79f3817ed7d2d609c7ba.1629621310.git.congdanhqx@gmail.com \
    --to=congdanhqx@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=thomas@slight.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.