Re: [PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules
From: Junio C Hamano <hidden>
Date: 2021-07-26 22:57:32
David Turner [off-list ref] writes:
If you have ever initialized a submodule, open_submodule will open it. If you then delete the submodule's worktree directory (but don't remove it from .gitmodules), git diff --submodule=diff would crash as it attempted to chdir into the now-deleted working tree directory.
Hmph. So what does the failure look like? The child process inside start_command() attempts chdir() and reports CHILD_ERR_CHDIR back, and we catch it as an error by reading from notify_pipe[0] and report failure from start_command()? Or are we talking about a more severe "crash" of an uncontrolled kind? Bypassing the execution of diff in the submodule like this patch does may avoid such a failure, but is that all we need to "fix" this issue? What does the user expect after removing a submodule that way and runs "diff" with the "--submodule=diff" option? Shouldn't we be giving "all lines from all files have been removed" patch? Thanks.
quoted hunk
Signed-off-by: David Turner <redacted> --- submodule.c | 3 ++ t/t4060-diff-submodule-option-diff-format.sh | 45 ++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 5 deletions(-)diff --git a/submodule.c b/submodule.c index 0b1d9c1dde..9031527a16 100644 --- a/submodule.c +++ b/submodule.c@@ -673,6 +673,9 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path, !(right || is_null_oid(two))) goto done; + if (!is_directory(path)) + goto done; + if (left) old_oid = one; if (right)diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index 69b9946931..10e330c08a 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh@@ -703,10 +703,26 @@ test_expect_success 'path filter' ' diff_cmp expected actual ' -commit_file sm2 +cat >.gitmodules <<-EOF +[submodule "sm2"] + path = sm2 + url = bogus_url +EOF +git add .gitmodules +commit_file sm2 .gitmodules + test_expect_success 'given commit' ' git diff-index -p --submodule=diff HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm2"] + +path = sm2 + +url = bogus_url Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 0000000...$head9 (new submodule) diff --git a/sm2/foo8 b/sm2/foo8@@ -728,15 +744,21 @@ test_expect_success 'given commit' ' ' test_expect_success 'setup .git file for sm2' ' - (cd sm2 && - REAL="$(pwd)/../.real" && - mv .git "$REAL" && - echo "gitdir: $REAL" >.git) + git submodule absorbgitdirs sm2 ' test_expect_success 'diff --submodule=diff with .git file' ' git diff --submodule=diff HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm2"] + +path = sm2 + +url = bogus_url Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 0000000...$head9 (new submodule) diff --git a/sm2/foo8 b/sm2/foo8@@ -757,6 +779,19 @@ test_expect_success 'diff --submodule=diff with .git file' ' diff_cmp expected actual ' +mv sm2 sm2-bak + +test_expect_success 'deleted submodule with .git file' ' + git diff-index -p --submodule=diff HEAD >actual && + cat >expected <<-EOF && + Submodule sm1 $head7...0000000 (submodule deleted) + Submodule sm2 $head9...0000000 (submodule deleted) + EOF + diff_cmp expected actual +' + +mv sm2-bak sm2 + test_expect_success 'setup nested submodule' ' git submodule add -f ./sm2 && git commit -a -m "add sm2" &&