Merge branch 'js/difftool-builtin'
authorJunio C Hamano <gitster@pobox.com>
Mon, 20 Mar 2017 20:56:00 +0000 (13:56 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 20 Mar 2017 20:56:00 +0000 (13:56 -0700)
"git difftool --dir-diff" used to die a controlled death giving a
"fatal" message when encountering a locally modified symbolic link,
but it started segfaulting since v2.12. This has been fixed.

* js/difftool-builtin:
difftool: handle modified symlinks in dir-diff mode
t7800: cleanup cruft left behind by tests
t7800: remove whitespace before redirect

builtin/difftool.c
t/t7800-difftool.sh
index d13350ce832cd8c0064c8a5b2d3c9be64f876b8d..25e54ad3edb7dfd7d5e5e9f1ded4dc508048ddc6 100644 (file)
@@ -254,6 +254,49 @@ static int ensure_leading_directories(char *path)
        }
 }
 
+/*
+ * Unconditional writing of a plain regular file is what
+ * "git difftool --dir-diff" wants to do for symlinks.  We are preparing two
+ * temporary directories to be fed to a Git-unaware tool that knows how to
+ * show a diff of two directories (e.g. "diff -r A B").
+ *
+ * Because the tool is Git-unaware, if a symbolic link appears in either of
+ * these temporary directories, it will try to dereference and show the
+ * difference of the target of the symbolic link, which is not what we want,
+ * as the goal of the dir-diff mode is to produce an output that is logically
+ * equivalent to what "git diff" produces.
+ *
+ * Most importantly, we want to get textual comparison of the result of the
+ * readlink(2).  get_symlink() provides that---it returns the contents of
+ * the symlink that gets written to a regular file to force the external tool
+ * to compare the readlink(2) result as text, even on a filesystem that is
+ * capable of doing a symbolic link.
+ */
+static char *get_symlink(const struct object_id *oid, const char *path)
+{
+       char *data;
+       if (is_null_oid(oid)) {
+               /* The symlink is unknown to Git so read from the filesystem */
+               struct strbuf link = STRBUF_INIT;
+               if (has_symlinks) {
+                       if (strbuf_readlink(&link, path, strlen(path)))
+                               die(_("could not read symlink %s"), path);
+               } else if (strbuf_read_file(&link, path, 128))
+                       die(_("could not read symlink file %s"), path);
+
+               data = strbuf_detach(&link, NULL);
+       } else {
+               enum object_type type;
+               unsigned long size;
+               data = read_sha1_file(oid->hash, &type, &size);
+               if (!data)
+                       die(_("could not read object %s for symlink %s"),
+                               oid_to_hex(oid), path);
+       }
+
+       return data;
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                        int argc, const char **argv)
 {
@@ -270,8 +313,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
        struct hashmap working_tree_dups, submodules, symlinks2;
        struct hashmap_iter iter;
        struct pair_entry *entry;
-       enum object_type type;
-       unsigned long size;
        struct index_state wtindex;
        struct checkout lstate, rstate;
        int rc, flags = RUN_GIT_CMD, err = 0;
@@ -377,13 +418,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                }
 
                if (S_ISLNK(lmode)) {
-                       char *content = read_sha1_file(loid.hash, &type, &size);
+                       char *content = get_symlink(&loid, src_path);
                        add_left_or_right(&symlinks2, src_path, content, 0);
                        free(content);
                }
 
                if (S_ISLNK(rmode)) {
-                       char *content = read_sha1_file(roid.hash, &type, &size);
+                       char *content = get_symlink(&roid, dst_path);
                        add_left_or_right(&symlinks2, dst_path, content, 1);
                        free(content);
                }
@@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                                return error("could not write '%s'", src_path);
                }
 
-               if (rmode) {
+               if (rmode && !S_ISLNK(rmode)) {
                        struct working_tree_entry *entry;
 
                        /* Avoid duplicate working_tree entries */
index 25241f40967ef1fb547771e64c69927de2f101ce..0e7f30db2d9116ec5d40b02612c97f765f3a4414 100755 (executable)
@@ -428,7 +428,7 @@ run_dir_diff_test 'difftool --dir-diff branch from subdirectory' '
                git difftool --dir-diff $symlinks --extcmd ls branch >output &&
                # "sub" must only exist in "right"
                # "file" and "file2" must be listed in both "left" and "right"
-               grep sub output > sub-output &&
+               grep sub output >sub-output &&
                test_line_count = 1 sub-output &&
                grep file"$" output >file-output &&
                test_line_count = 2 file-output &&
@@ -591,6 +591,7 @@ test_expect_success 'difftool --no-symlinks detects conflict ' '
 '
 
 test_expect_success 'difftool properly honors gitlink and core.worktree' '
+       test_when_finished rm -rf submod/ule &&
        git submodule add ./. submod/ule &&
        test_config -C submod/ule diff.tool checktrees &&
        test_config -C submod/ule difftool.checktrees.cmd '\''
@@ -600,11 +601,13 @@ test_expect_success 'difftool properly honors gitlink and core.worktree' '
                cd submod/ule &&
                echo good >expect &&
                git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
-               test_cmp expect actual
+               test_cmp expect actual &&
+               rm -f expect actual
        )
 '
 
 test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '
+       test_when_finished git reset --hard &&
        git init dirlinks &&
        (
                cd dirlinks &&
@@ -623,4 +626,64 @@ test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '
        )
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
+       test_when_finished git reset --hard &&
+       touch b &&
+       ln -s b c &&
+       git add b c &&
+       test_tick &&
+       git commit -m initial &&
+       touch d &&
+       rm c &&
+       ln -s d c &&
+       cat >expect <<-EOF &&
+               b
+               c
+
+               c
+       EOF
+       git difftool --symlinks --dir-diff --extcmd ls >output &&
+       grep -v ^/ output >actual &&
+       test_cmp expect actual &&
+
+       git difftool --no-symlinks --dir-diff --extcmd ls >output &&
+       grep -v ^/ output >actual &&
+       test_cmp expect actual &&
+
+       # The left side contains symlink "c" that points to "b"
+       test_config difftool.cat.cmd "cat \$LOCAL/c" &&
+       printf "%s\n" b >expect &&
+
+       git difftool --symlinks --dir-diff --tool cat >actual &&
+       test_cmp expect actual &&
+
+       git difftool --symlinks --no-symlinks --dir-diff --tool cat >actual &&
+       test_cmp expect actual &&
+
+       # The right side contains symlink "c" that points to "d"
+       test_config difftool.cat.cmd "cat \$REMOTE/c" &&
+       printf "%s\n" d >expect &&
+
+       git difftool --symlinks --dir-diff --tool cat >actual &&
+       test_cmp expect actual &&
+
+       git difftool --no-symlinks --dir-diff --tool cat >actual &&
+       test_cmp expect actual &&
+
+       # Deleted symlinks
+       rm -f c &&
+       cat >expect <<-EOF &&
+               b
+               c
+
+       EOF
+       git difftool --symlinks --dir-diff --extcmd ls >output &&
+       grep -v ^/ output >actual &&
+       test_cmp expect actual &&
+
+       git difftool --no-symlinks --dir-diff --extcmd ls >output &&
+       grep -v ^/ output >actual &&
+       test_cmp expect actual
+'
+
 test_done