{"thread":{"id":"45382","subject":"[PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode","startedAt":"2017-03-15T09:33:15Z","lastAt":"2017-03-15T20:58:35Z","messageCount":3,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"314126","messageId":"20170315093130.30110-1-davvid@gmail.com","threadId":"45382","inReplyTo":null,"subject":"[PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-15T09:31:30Z","receivedAt":"2017-03-15T09:33:15Z","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---\nOnly 3/3 was re-sent; the rest are the same.\n\nWhen re-reading the patch I noticed two spots where spurious\nwhitespace was added.  I've dropped those hunks.\n\n builtin/difftool.c  | 51 ++++++++++++++++++++++++++++++++++++++++-----\n t/t7800-difftool.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex d13350ce83..25e54ad3ed 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 */\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.g054a7c81e7\n\n"},{"id":"314157","messageId":"xmqq8to6fk15.fsf@gitster.mtv.corp.google.com","threadId":"45382","inReplyTo":"20170315093130.30110-1-davvid@gmail.com","subject":"Re: [PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T18:54:14Z","receivedAt":"2017-03-15T18:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\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> Only 3/3 was re-sent; the rest are the same.\n\nOK, so in short you did the get_symlink() thing that was brought up\nin the previous round of review and everything else fell out as a\nnatural consequence?  That is wonderful ;-)\n\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\nIt still makes me wonder why the new !S_ISLNK() is done only for the\nright hand side, though.  In fact, the processing done for the left\nhand side and the right hand side are vastly different even without\nthis patch.\n\nI suspect this is probably because the code is not prepared to drive\nthe underlying \"diff\" when given the \"-R\" option (if I am reading\nthe code correctly, argv[] that came from the end-user is appended\nto \"diff --raw --no-abbrev -z\", so the user could ask \"difftool\n--dir-diff -R ...\"), in which case you would see the working tree\nfiles as the left hand side of the diff.  In the dir-diff mode,\nbecause you want to make only the working-tree side writable (and\nreflect whatever edit the user made back to the working-tree side),\nthe choices you have to fix it would either be forbid \"-R\" (which is\nless preferrable as it is a more brittle solution between the two)\nor read the \"diff --raw\" output and swap the sides when you notice\nthat LHS has 0{40} with non 0 mode, which is a sign that that side\nrepresents the working tree.\n\nHaving said all that, let's focus on the \"symlink\" stuff in this\nseries.\n\nThanks.\n"},{"id":"314172","messageId":"20170315205735.5c6i4uivzj77bfsb@gmail.com","threadId":"45382","inReplyTo":"xmqq8to6fk15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-15T20:58:15Z","receivedAt":"2017-03-15T20:58:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 15, 2017 at 11:54:14AM -0700, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \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> It still makes me wonder why the new !S_ISLNK() is done only for the\n> right hand side, though.  In fact, the processing done for the left\n> hand side and the right hand side are vastly different even without\n> this patch.\n> \n> I suspect this is probably because the code is not prepared to drive\n> the underlying \"diff\" when given the \"-R\" option (if I am reading\n> the code correctly, argv[] that came from the end-user is appended\n> to \"diff --raw --no-abbrev -z\", so the user could ask \"difftool\n> --dir-diff -R ...\"), in which case you would see the working tree\n> files as the left hand side of the diff.  In the dir-diff mode,\n> because you want to make only the working-tree side writable (and\n> reflect whatever edit the user made back to the working-tree side),\n> the choices you have to fix it would either be forbid \"-R\" (which is\n> less preferrable as it is a more brittle solution between the two)\n> or read the \"diff --raw\" output and swap the sides when you notice\n> that LHS has 0{40} with non 0 mode, which is a sign that that side\n> represents the working tree.\n> \n> Having said all that, let's focus on the \"symlink\" stuff in this\n> series.\n> \n> Thanks.\n\nAgreed.  I can take a look at the reverse-diff\nquestion later this week separately from this issue.\n\nThe symlink test case I just added can be used as a starting\nground for adding reverse-diff tests.\n\nThanks Junio and Dscho for the reviews,\n-- \nDavid\n"}]}