{"thread":{"id":"45381","subject":"[PATCH 1/3] t7800: remove whitespace before redirect","startedAt":"2017-03-15T06:54:16Z","lastAt":"2017-03-15T16:42:25Z","messageCount":4,"participants":["David Aguilar","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"314118","messageId":"20170315065406.6739-1-davvid@gmail.com","threadId":"45381","inReplyTo":null,"subject":"[PATCH 1/3] t7800: remove whitespace before redirect","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-15T06:54:04Z","receivedAt":"2017-03-15T06:54:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar <davvid@gmail.com>\n---\nCleanup before the fix.\n\n t/t7800-difftool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 25241f4096..e1ec292718 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -428,7 +428,7 @@ run_dir_diff_test 'difftool --dir-diff branch from subdirectory' '\n \t\tgit difftool --dir-diff $symlinks --extcmd ls branch >output &&\n \t\t# \"sub\" must only exist in \"right\"\n \t\t# \"file\" and \"file2\" must be listed in both \"left\" and \"right\"\n-\t\tgrep sub output > sub-output &&\n+\t\tgrep sub output >sub-output &&\n \t\ttest_line_count = 1 sub-output &&\n \t\tgrep file\"$\" output >file-output &&\n \t\ttest_line_count = 2 file-output &&\n-- \n2.12.0.309.gffef9e61c2\n\n"},{"id":"314119","messageId":"20170315065406.6739-2-davvid@gmail.com","threadId":"45381","inReplyTo":"20170315065406.6739-1-davvid@gmail.com","subject":"[PATCH 2/3] t7800: cleanup cruft left behind by tests","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-15T06:54:05Z","receivedAt":"2017-03-15T06:54:18Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar <davvid@gmail.com>\n---\nMore cleanup, this is needed because the final patch adds a test\nto t7800 that was sensitive to the cruft left behind.\n\n t/t7800-difftool.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e1ec292718..e0e65df8de 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -591,6 +591,7 @@ test_expect_success 'difftool --no-symlinks detects conflict ' '\n '\n \n test_expect_success 'difftool properly honors gitlink and core.worktree' '\n+\ttest_when_finished rm -rf submod/ule &&\n \tgit submodule add ./. submod/ule &&\n \ttest_config -C submod/ule diff.tool checktrees &&\n \ttest_config -C submod/ule difftool.checktrees.cmd '\\''\n@@ -600,11 +601,13 @@ test_expect_success 'difftool properly honors gitlink and core.worktree' '\n \t\tcd submod/ule &&\n \t\techo good >expect &&\n \t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp expect actual &&\n+\t\trm -f expect actual\n \t)\n '\n \n test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n+\ttest_when_finished git reset --hard &&\n \tgit init dirlinks &&\n \t(\n \t\tcd dirlinks &&\n-- \n2.12.0.309.gffef9e61c2\n\n"},{"id":"314120","messageId":"20170315065406.6739-3-davvid@gmail.com","threadId":"45381","inReplyTo":"20170315065406.6739-1-davvid@gmail.com","subject":"[PATCH 3/3] difftool: handle modified symlinks in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-15T06:54:06Z","receivedAt":"2017-03-15T06:54:20Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Detect the null object ID for symlinks in dir-diff so that difftool can\ndetect when symlinks are modified in the worktree.\n\nPreviously, a null symlink object ID would crash difftool.\nHandle null object IDs as unknown content that must be read from\nthe worktree.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis was reworked a bit since the original patch.\nThe subject line changed, a lot of comments were added,\nand the tests were made more extensive.\n\nThis implementation is simpler since we now use a get_symlink()\nfunction in one place and simply skip the problematic code path\nthat does not apply to symlinks.  Existing code handles writing\nthe symlink content so write_symlink_file() from the original\npatch is gone.\n\n builtin/difftool.c  | 53 +++++++++++++++++++++++++++++++++++++++++-----\n t/t7800-difftool.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 108 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex d13350ce83..6aab5cd23a 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -254,6 +254,49 @@ static int ensure_leading_directories(char *path)\n \t}\n }\n \n+/*\n+ * Unconditional writing of a plain regular file is what\n+ * \"git difftool --dir-diff\" wants to do for symlinks.  We are preparing two\n+ * temporary directories to be fed to a Git-unaware tool that knows how to\n+ * show a diff of two directories (e.g. \"diff -r A B\").\n+ *\n+ * Because the tool is Git-unaware, if a symbolic link appears in either of\n+ * these temporary directories, it will try to dereference and show the\n+ * difference of the target of the symbolic link, which is not what we want,\n+ * as the goal of the dir-diff mode is to produce an output that is logically\n+ * equivalent to what \"git diff\" produces.\n+ *\n+ * Most importantly, we want to get textual comparison of the result of the\n+ * readlink(2).  get_symlink() provides that---it returns the contents of\n+ * the symlink that gets written to a regular file to force the external tool\n+ * to compare the readlink(2) result as text, even on a filesystem that is\n+ * capable of doing a symbolic link.\n+ */\n+static char *get_symlink(const struct object_id *oid, const char *path)\n+{\n+\tchar *data;\n+\tif (is_null_oid(oid)) {\n+\t\t/* The symlink is unknown to Git so read from the filesystem */\n+\t\tstruct strbuf link = STRBUF_INIT;\n+\t\tif (has_symlinks) {\n+\t\t\tif (strbuf_readlink(&link, path, strlen(path)))\n+\t\t\t\tdie(_(\"could not read symlink %s\"), path);\n+\t\t} else if (strbuf_read_file(&link, path, 128))\n+\t\t\tdie(_(\"could not read symlink file %s\"), path);\n+\n+\t\tdata = strbuf_detach(&link, NULL);\n+\t} else {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tdata = read_sha1_file(oid->hash, &type, &size);\n+\t\tif (!data)\n+\t\t\tdie(_(\"could not read object %s for symlink %s\"),\n+\t\t\t\toid_to_hex(oid), path);\n+\t}\n+\n+\treturn data;\n+}\n+\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tint argc, const char **argv)\n {\n@@ -270,8 +313,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct hashmap working_tree_dups, submodules, symlinks2;\n \tstruct hashmap_iter iter;\n \tstruct pair_entry *entry;\n-\tenum object_type type;\n-\tunsigned long size;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n \tint rc, flags = RUN_GIT_CMD, err = 0;\n@@ -377,13 +418,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t}\n \n \t\tif (S_ISLNK(lmode)) {\n-\t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n+\t\t\tchar *content = get_symlink(&loid, src_path);\n \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n \t\t\tfree(content);\n \t\t}\n \n \t\tif (S_ISLNK(rmode)) {\n-\t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n+\t\t\tchar *content = get_symlink(&roid, dst_path);\n \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n \t\t\tfree(content);\n \t\t}\n@@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\treturn error(\"could not write '%s'\", src_path);\n \t\t}\n \n-\t\tif (rmode) {\n+\t\tif (rmode && !S_ISLNK(rmode)) {\n \t\t\tstruct working_tree_entry *entry;\n \n \t\t\t/* Avoid duplicate working_tree entries */\n@@ -414,6 +455,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\toidcpy(&ce->oid, &roid);\n \t\t\t\tstrcpy(ce->name, dst_path);\n \t\t\t\tce->ce_namelen = dst_path_len;\n+\n \t\t\t\tif (checkout_entry(ce, &rstate, NULL))\n \t\t\t\t\treturn error(\"could not write '%s'\",\n \t\t\t\t\t\t     dst_path);\n@@ -487,6 +529,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t * shows only the link itself, not the contents of the link target.\n \t * This loop replicates that behavior.\n \t */\n+\n \thashmap_iter_init(&symlinks2, &iter);\n \twhile ((entry = hashmap_iter_next(&iter))) {\n \t\tif (*entry->left) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e0e65df8de..0e7f30db2d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -626,4 +626,64 @@ test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n \t)\n '\n \n+test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n+\ttest_when_finished git reset --hard &&\n+\ttouch b &&\n+\tln -s b c &&\n+\tgit add b c &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\ttouch d &&\n+\trm c &&\n+\tln -s d c &&\n+\tcat >expect <<-EOF &&\n+\t\tb\n+\t\tc\n+\n+\t\tc\n+\tEOF\n+\tgit difftool --symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# The left side contains symlink \"c\" that points to \"b\"\n+\ttest_config difftool.cat.cmd \"cat \\$LOCAL/c\" &&\n+\tprintf \"%s\\n\" b >expect &&\n+\n+\tgit difftool --symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --symlinks --no-symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# The right side contains symlink \"c\" that points to \"d\"\n+\ttest_config difftool.cat.cmd \"cat \\$REMOTE/c\" &&\n+\tprintf \"%s\\n\" d >expect &&\n+\n+\tgit difftool --symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --tool cat >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Deleted symlinks\n+\trm -f c &&\n+\tcat >expect <<-EOF &&\n+\t\tb\n+\t\tc\n+\n+\tEOF\n+\tgit difftool --symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit difftool --no-symlinks --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.12.0.309.gffef9e61c2\n\n"},{"id":"314144","messageId":"alpine.DEB.2.20.1703151739070.3767@virtualbox","threadId":"45381","inReplyTo":"20170315065406.6739-3-davvid@gmail.com","subject":"Re: [PATCH 3/3] difftool: handle modified symlinks in dir-diff mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-15T16:41:06Z","receivedAt":"2017-03-15T16:42:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nOn Tue, 14 Mar 2017, David Aguilar wrote:\n\n> Detect the null object ID for symlinks in dir-diff so that difftool can\n> detect when symlinks are modified in the worktree.\n> \n> Previously, a null symlink object ID would crash difftool.\n> Handle null object IDs as unknown content that must be read from\n> the worktree.\n> \n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> This was reworked a bit since the original patch.\n> The subject line changed, a lot of comments were added,\n> and the tests were made more extensive.\n> \n> This implementation is simpler since we now use a get_symlink()\n> function in one place and simply skip the problematic code path\n> that does not apply to symlinks.  Existing code handles writing\n> the symlink content so write_symlink_file() from the original\n> patch is gone.\n\nI really like this version. It is very easy to follow, the extensive\ncomments make a lot of sense.\n\nWell done!\nDscho\n\n"}]}