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

David Turner [off-list ref] writes:

quoted

From: Junio C Hamano <redacted>
Sent: Monday, July 26, 2021 6:57 PM
To: David Turner <redacted>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/3] diff --submodule=diff: do not fail on 
ever-initialied deleted submodules

David Turner [off-list ref] writes:

quoted

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?

It's the first kind.

quoted

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?

Just a note: this only matters if the submodules git dir is
absorbed.  If not, then we no longer have anywhere to run the
diff.  But that case does not trigger this error, because in that
case, open_submodule fails, so we don't resolve a left commit, so
we exit early, which is the only thing we could do.

If absorbed, then we could, in theory, go into the submodule's
absorbed git dir (.git/modules/sm2) and run the diff there.  But
in practice, that's a bit more complicated, because `git diff`
expects to be run from inside a working directory, not a git dir.
So it looks in the config for core.worktree, and does
chdir("../../../sm2"), which is the very dir that we're trying to
avoid visiting because it's been deleted.  We could work around
this by setting GIT_WORK_TREE (and GIT_DIR) to ".".  This
actually seems to work, but it's a little weird to set
GIT_WORK_TREE to something that is not a working tree just to
avoid an unnecessary chdir.  To my mild surprise, it also seems
to work correctly in the case of deleted nested (absorbed)
submodules.  What do you think of this idea?

(Side note: The same bit about chdir into the working tree is
true for diff-tree even though it normally does not need anything
from the working tree.  I say "normally", because in the case of
nested submodules, it might need to look inside those submodules,
which might themselves not be absorbed.  Of course, this case
cannot obtain if the submodule in the worktree has been deleted.
We should consider fixing the docs for git diff-tree
--submodule=diff to specify that it only works if -p is passed.)

(Sorry if the formatting on this email ends up bad -- corporate
email is... corporate .  I'm going to CC my personal address so
that I can use a better mailer on future replies). 

That I had to ask questions based on the proposed log message and
you needed to answer to clarify means that the next person who
encounters this commit in "git log" would likely have to ask the
same question, and worse, unlike I had you, there is nobody to whom
they ask for help understanding this commit.

Thanks.