difftool: --no-index is incompatible with --dir-diff

While debugging why a straight-forward backport of the js/difftool-no-index branch onto Git for Windows' master does not work as expected (spoiler: it's due to git diff --no-index now implying --ext-diff, a change that is not yet in Git for Windows' master), I ran into quite a few segmentation faults while trying to make --no-index work nicely with --dir-diff.

And this was a much bigger project than I thought it would be: the worktree is NULL and causes a segmentation fault, make_transient_cache_entry() returns NULL instead of a cache entry, while looking up replacement objects there is a crash because git_replace_ref_base is NULL, during checkout_entry()'s life time, the prepare_alt_odb() function tries to dereference the_repository.objects->odb (which is NULL, of course). I'll stop here with all the segmentation faults.

Even worse, when trying to compare ../1 to ../2 (as we do in the regression test for difftool --no-index outside any repository), we run afoul of verify_path() complaining that this is not a valid path for an index entry.

After figuring all of that out, I took a step back and guess what: it turns out that it does not even make sense to combine those two options. The --dir-diff option exists to enable diff'ing subdirectories of a worktree while pretending that untracked and ignored files in them do not even exist. And without a worktree, there simply are no untracked or ignored files, so --dir-diff is not necessary.

Still, we should address those segmentation faults. And I think we address them best by explicitly telling the user that they cannot use those two options together.

Johannes Schindelin (1):
difftool --no-index: error out on --dir-diff (and don't crash)

 builtin/difftool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

base-commit: 20de316e33446f37200e51aa333ba7d824dfd478
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-186%2Fdscho%2Fdifftool-no-index-extra-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-186/dscho/difftool-no-index-extra-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/186

gitgitgadget

From: Johannes Schindelin <[email protected]>

In --no-index mode, we now no longer require a worktree nor a repository. But some code paths in difftool expect those to be present.

The most notable such code path is the --dir-diff one: we use the existing checkout machinery to copy the files, and that machinery looks up replacement refs, looks at alternate ODBs, wants to use the worktree path, etc.

Rather than running into segmentation faults, let's die with an informative error message.

---
 builtin/difftool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 4fff1e83f9..5704a76088 100644
@@ -735,7 +735,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		setup_work_tree();
 		setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
-	}
+	} else if (dir_diff)
+		die(_("--dir-diff is incompatible with --no-index"));
 
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hmph. So there is no point in using the dir-diff mode when you have say two extracts of pristine tarballs?

I guess that's true.

The difftool will launch an external "directory diff tool" on two directories populated with the two "things" (be it "the contents from a tree-ish", "the working tree files", "the contents in the index") in the end, but you already have two directories to be compared when you run "diff --no-index", so there is no point in going through the "difftool" middleman. You can just feed the two directories you have directly to that external directory diff tool.