Re: [PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules

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

David Turner <dturner@twosigma.com> 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.

> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  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" &&