{"thread":{"id":"56339","subject":"[PATCH v2] diff-lib: ignore all outsider if --relative asked","startedAt":"2021-08-21T04:04:12Z","lastAt":"2021-08-22T08:49:41Z","messageCount":2,"participants":["Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"433300","messageId":"0d73c7181969d2916d71d7aeb6d788324a0db68b.1629514355.git.congdanhqx@gmail.com","threadId":"56339","inReplyTo":null,"subject":"[PATCH v2] diff-lib: ignore all outsider if --relative asked","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-21T04:03:54Z","receivedAt":"2021-08-21T04:04:12Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"For diff family commands, we can tell them to exclude changes outside\nof some directories if --relative is requested.\n\nIn diff_unmerge(), NULL will be returned if the requested path is\noutside of the interesting directories, thus we'll run into NULL\npointer dereference in run_diff_files when trying to dereference\nits return value.\n\nChecking for return value of diff_unmerge before dereferencing\nis not sufficient, though. Since, diff engine will try to work on such\npathspec later.\n\nLet's not run diff on those unintesting entries, instead.\nAs a side effect, by skipping like that, we can save some CPU cycles.\n\nReported-by: Thomas De Zeeuw <thomas@slight.dev>\nTested-by: Carlo Arenas <carenas@gmail.com>\nSigned-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n---\nRange-diff against v1:\n1:  57a9edc3af ! 1:  0d73c71819 diff-lib: ignore all outsider if --relative asked\n    @@ Commit message\n         pointer dereference in run_diff_files when trying to dereference\n         its return value.\n     \n    -    We can simply check for NULL there before dereferencing said\n    -    return value.  However, we can do better by not running diff\n    -    on those unintesting entries.  Let's do that instead.\n    +    Checking for return value of diff_unmerge before dereferencing\n    +    is not sufficient, though. Since, diff engine will try to work on such\n    +    pathspec later.\n    +\n    +    Let's not run diff on those unintesting entries, instead.\n    +    As a side effect, by skipping like that, we can save some CPU cycles.\n     \n         Reported-by: Thomas De Zeeuw <thomas@slight.dev>\n    +    Tested-by: Carlo Arenas <carenas@gmail.com>\n         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n     \n    -\n    - ## Notes ##\n    -    Check for return value of diff_unmerge is not enough.\n    -\n    -    Yes, it works with --name-only, however, with only --relative,\n    -    git-diff shows unmerged entries outside of subdir, too.\n    -\n    -    Furthermore, the filename in \"diff --cc\" ignores the relative prefix.\n    -    Fixing this requires touching all over places, at least from my study.\n    -    Let's fix the crash, first.\n    -\n    -    We have two choices here:\n    -\n    -    * Check pair, aka return value of diff_unmerge, like my original\n    -      suggestion, and the unmerged entries from outside will be shown, too.\n    -      Some inconsistent will be observed, --name-only won't list files\n    -      outside of subdir, while the patch shows them.  At least, it doesn't\n    -      create false impression of no change outside of subdir.\n    -\n    -    * Skip all outsiders, like this patch.\n    -\n    -    While I prefer this approach, I don't know all ramifications of this change,\n    -    let's say an entry moved to outside of subdir in one side, and modified in\n    -    other side.\n    -\n    -    Because, I pick the different approach, Junio's ack isn't included here.\n    -\n    -    Cc: Junio C Hamano <gitster@pobox.com>\n    -\n      ## diff-lib.c ##\n     @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n      \t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n\n diff-lib.c               |  4 +++\n t/t4045-diff-relative.sh | 53 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f9eadc4fc1..ca085a03ef 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -117,6 +117,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n \t\t\tcontinue;\n \n+\t\tif (revs->diffopt.prefix &&\n+\t\t    strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length))\n+\t\t\tcontinue;\n+\n \t\tif (ce_stage(ce)) {\n \t\t\tstruct combine_diff_path *dpath;\n \t\t\tstruct diff_filepair *pair;\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 61ba5f707f..8cbbe53262 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -162,4 +162,57 @@ check_diff_relative_option subdir file2 true --no-relative --relative\n check_diff_relative_option . file2 false --no-relative --relative=subdir\n check_diff_relative_option . file2 true --no-relative --relative=subdir\n \n+test_expect_success 'setup diff --relative unmerged' '\n+\ttest_commit zero file0 &&\n+\ttest_commit base subdir/file0 &&\n+\tgit switch -c br1 &&\n+\ttest_commit one file0 &&\n+\ttest_commit sub1 subdir/file0 &&\n+\tgit switch -c br2 base &&\n+\ttest_commit two file0 &&\n+\tgit switch -c br3 &&\n+\ttest_commit sub3 subdir/file0\n+'\n+\n+test_expect_success 'diff --relative without change in subdir' '\n+\tgit switch br2 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge one &&\n+\tgit -C subdir diff --relative >out &&\n+\ttest_must_be_empty out &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_must_be_empty out\n+'\n+\n+test_expect_success 'diff --relative --name-only with change in subdir' '\n+\tgit switch br3 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge sub1 &&\n+\ttest_write_lines file0 file0 >expected &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_failure 'diff --relative with change in subdir' '\n+\tgit switch br3 &&\n+\tbr1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&\n+\tbr3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge br1 &&\n+\tcat >expected <<-EOF &&\n+\tdiff --cc file0\n+\tindex $br3_blob,$br1_blob..0000000\n+\t--- a/file0\n+\t+++ b/file0\n+\t@@@ -1,1 -1,1 +1,5 @@@\n+\t++<<<<<<< HEAD\n+\t +sub3\n+\t++=======\n+\t+ sub1\n+\t++>>>>>>> sub1\n+\tEOF\n+\tgit -C subdir diff --relative >out &&\n+\ttest_cmp expected out\n+'\n+\n test_done\n-- \n2.33.0.254.g68ee769121\n\n"},{"id":"433317","messageId":"a140b292f94de9f4d96f79f3817ed7d2d609c7ba.1629621310.git.congdanhqx@gmail.com","threadId":"56339","inReplyTo":"0d73c7181969d2916d71d7aeb6d788324a0db68b.1629514355.git.congdanhqx@gmail.com","subject":"[PATCH v3] diff-lib: ignore all outsider if --relative asked","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-22T08:49:08Z","receivedAt":"2021-08-22T08:49:41Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"For diff family commands, we can tell them to exclude changes outside\nof some directories if --relative is requested.\n\nIn diff_unmerge(), NULL will be returned if the requested path is\noutside of the interesting directories, thus we'll run into NULL\npointer dereference in run_diff_files when trying to dereference\nits return value.\n\nChecking for return value of diff_unmerge before dereferencing\nis not sufficient, though. Since, diff engine will try to work on such\npathspec later.\n\nLet's not run diff on those unintesting entries, instead.\nAs a side effect, by skipping like that, we can save some CPU cycles.\n\nReported-by: Thomas De Zeeuw <thomas@slight.dev>\nTested-by: Carlo Arenas <carenas@gmail.com>\nSigned-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n---\n\nSorry for the noise, I need to send v3, because:\n\nChange in v3 from v2:\n Fix the conflict marker in failure test. Originally, I wrote the test with:\n \"git merge sub1\" not \"git merge br1\"\n\nRange-diff against v2:\n1:  0d73c71819 ! 1:  a140b292f9 diff-lib: ignore all outsider if --relative asked\n    @@ t/t4045-diff-relative.sh: check_diff_relative_option subdir file2 true --no-rela\n     +\t +sub3\n     +\t++=======\n     +\t+ sub1\n    -+\t++>>>>>>> sub1\n    ++\t++>>>>>>> br1\n     +\tEOF\n     +\tgit -C subdir diff --relative >out &&\n     +\ttest_cmp expected out\n\nRange-diff of v2 against v1:\n1:  57a9edc3af ! 1:  0d73c71819 diff-lib: ignore all outsider if --relative asked\n    @@ Commit message\n         pointer dereference in run_diff_files when trying to dereference\n         its return value.\n     \n    -    We can simply check for NULL there before dereferencing said\n    -    return value.  However, we can do better by not running diff\n    -    on those unintesting entries.  Let's do that instead.\n    +    Checking for return value of diff_unmerge before dereferencing\n    +    is not sufficient, though. Since, diff engine will try to work on such\n    +    pathspec later.\n    +\n    +    Let's not run diff on those unintesting entries, instead.\n    +    As a side effect, by skipping like that, we can save some CPU cycles.\n     \n         Reported-by: Thomas De Zeeuw <thomas@slight.dev>\n    +    Tested-by: Carlo Arenas <carenas@gmail.com>\n         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n     \n diff-lib.c               |  4 +++\n t/t4045-diff-relative.sh | 53 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f9eadc4fc1..ca085a03ef 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -117,6 +117,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n \t\t\tcontinue;\n \n+\t\tif (revs->diffopt.prefix &&\n+\t\t    strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length))\n+\t\t\tcontinue;\n+\n \t\tif (ce_stage(ce)) {\n \t\t\tstruct combine_diff_path *dpath;\n \t\t\tstruct diff_filepair *pair;\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 61ba5f707f..fab351b48a 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -162,4 +162,57 @@ check_diff_relative_option subdir file2 true --no-relative --relative\n check_diff_relative_option . file2 false --no-relative --relative=subdir\n check_diff_relative_option . file2 true --no-relative --relative=subdir\n \n+test_expect_success 'setup diff --relative unmerged' '\n+\ttest_commit zero file0 &&\n+\ttest_commit base subdir/file0 &&\n+\tgit switch -c br1 &&\n+\ttest_commit one file0 &&\n+\ttest_commit sub1 subdir/file0 &&\n+\tgit switch -c br2 base &&\n+\ttest_commit two file0 &&\n+\tgit switch -c br3 &&\n+\ttest_commit sub3 subdir/file0\n+'\n+\n+test_expect_success 'diff --relative without change in subdir' '\n+\tgit switch br2 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge one &&\n+\tgit -C subdir diff --relative >out &&\n+\ttest_must_be_empty out &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_must_be_empty out\n+'\n+\n+test_expect_success 'diff --relative --name-only with change in subdir' '\n+\tgit switch br3 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge sub1 &&\n+\ttest_write_lines file0 file0 >expected &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_failure 'diff --relative with change in subdir' '\n+\tgit switch br3 &&\n+\tbr1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&\n+\tbr3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge br1 &&\n+\tcat >expected <<-EOF &&\n+\tdiff --cc file0\n+\tindex $br3_blob,$br1_blob..0000000\n+\t--- a/file0\n+\t+++ b/file0\n+\t@@@ -1,1 -1,1 +1,5 @@@\n+\t++<<<<<<< HEAD\n+\t +sub3\n+\t++=======\n+\t+ sub1\n+\t++>>>>>>> br1\n+\tEOF\n+\tgit -C subdir diff --relative >out &&\n+\ttest_cmp expected out\n+'\n+\n test_done\n-- \n2.33.0.254.g68ee769121\n\n"}]}