{"thread":{"id":"51057","subject":"[PATCH 0/1] difftool: --no-index is incompatible with --dir-diff","startedAt":"2019-05-08T21:52:44Z","lastAt":"2019-05-09T07:29:23Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"375187","messageId":"pull.186.git.gitgitgadget@gmail.com","threadId":"51057","inReplyTo":null,"subject":"[PATCH 0/1] difftool: --no-index is incompatible with --dir-diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-08T21:52:40Z","receivedAt":"2019-05-08T21:52:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While debugging why a straight-forward backport of the js/difftool-no-index \nbranch onto Git for Windows' master does not work as expected (spoiler: it's\ndue to git diff --no-index now implying --ext-diff, a change that is not yet\nin Git for Windows' master), I ran into quite a few segmentation faults\nwhile trying to make --no-index work nicely with --dir-diff.\n\nAnd this was a much bigger project than I thought it would be: the worktree \nis NULL and causes a segmentation fault, make_transient_cache_entry() \nreturns NULL instead of a cache entry, while looking up replacement objects\nthere is a crash because git_replace_ref_base is NULL, during \ncheckout_entry()'s life time, the prepare_alt_odb() function tries to\ndereference the_repository.objects->odb (which is NULL, of course). I'll\nstop here with all the segmentation faults.\n\nEven worse, when trying to compare ../1 to ../2 (as we do in the regression\ntest for difftool --no-index outside any repository), we run afoul of \nverify_path() complaining that this is not a valid path for an index entry.\n\nAfter figuring all of that out, I took a step back and guess what: it turns\nout that it does not even make sense to combine those two options. The \n--dir-diff option exists to enable diff'ing subdirectories of a worktree\nwhile pretending that untracked and ignored files in them do not even exist.\nAnd without a worktree, there simply are no untracked or ignored files, so \n--dir-diff is not necessary.\n\nStill, we should address those segmentation faults. And I think we address\nthem best by explicitly telling the user that they cannot use those two\noptions together.\n\nJohannes Schindelin (1):\n difftool --no-index: error out on --dir-diff (and don't crash)\n\n builtin/difftool.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\n\nbase-commit: 20de316e33446f37200e51aa333ba7d824dfd478\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-186%2Fdscho%2Fdifftool-no-index-extra-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-186/dscho/difftool-no-index-extra-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/186\n-- \ngitgitgadget\n"},{"id":"375188","messageId":"9f2ee5ef0b84b348d07fedc47fced8d4772a9c43.1557352359.git.gitgitgadget@gmail.com","threadId":"51057","inReplyTo":"pull.186.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] difftool --no-index: error out on --dir-diff (and don't crash)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-08T21:52:41Z","receivedAt":"2019-05-08T21:52:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn `--no-index` mode, we now no longer require a worktree nor a\nrepository. But some code paths in `difftool` expect those to be\npresent.\n\nThe most notable such code path is the `--dir-diff` one: we use the\nexisting checkout machinery to copy the files, and that machinery looks\nup replacement refs, looks at alternate ODBs, wants to use the worktree\npath, etc.\n\nRather than running into segmentation faults, let's die with an\ninformative error message.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/difftool.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4fff1e83f9..5704a76088 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -735,7 +735,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tsetup_work_tree();\n \t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n-\t}\n+\t} else if (dir_diff)\n+\t\tdie(_(\"--dir-diff is incompatible with --no-index\"));\n \n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n-- \ngitgitgadget\n"},{"id":"375203","messageId":"xmqqv9ykdr1w.fsf@gitster-ct.c.googlers.com","threadId":"51057","inReplyTo":"pull.186.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] difftool: --no-index is incompatible with --dir-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-09T07:29:15Z","receivedAt":"2019-05-09T07:29:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> ... it does not even make sense to combine those two options. The \n> --dir-diff option exists to enable diff'ing subdirectories of a worktree\n> while pretending that untracked and ignored files in them do not even exist.\n\nHmph. So there is no point in using the dir-diff mode when you have\nsay two extracts of pristine tarballs?\n\nI guess that's true.\n\nThe difftool will launch an external \"directory diff tool\" on two\ndirectories populated with the two \"things\" (be it \"the contents\nfrom a tree-ish\", \"the working tree files\", \"the contents in the\nindex\") in the end, but you already have two directories to be\ncompared when you run \"diff --no-index\", so there is no point in\ngoing through the \"difftool\" middleman. You can just feed the two\ndirectories you have directly to that external directory diff tool.\n\n> base-commit: 20de316e33446f37200e51aa333ba7d824dfd478\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-186%2Fdscho%2Fdifftool-no-index-extra-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-186/dscho/difftool-no-index-extra-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/186\n"}]}