{"thread":{"id":"32203","subject":"difftool -d symlinks, under what conditions","startedAt":"2012-11-26T20:23:16Z","lastAt":"2013-03-14T22:31:53Z","messageCount":50,"participants":["Matt McClure","David Aguilar","John Keeping","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"203915","messageId":"CAJELnLGq_oLBiNHANoaE7iEiA6g4fXX0PtJbqPFi4PQ+5LLvnA@mail.gmail.com","threadId":"32203","inReplyTo":null,"subject":"difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2012-11-26T20:23:16Z","receivedAt":"2012-11-26T20:23:16Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"I'm finding the behavior of `git difftool -d` surprising. It seems that it\nonly uses symlinks to the working copy for files that are modified in the\nworking copy since the most recent commit. I would have expected it to use\nsymlinks for all files whose version under comparison is the working copy\nversion, regardless of whether the working copy differs from the HEAD.\n\nI'm using\n\n $ git --version\n git version 1.8.0\n\non a Mac from Homebrew.\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"203967","messageId":"CAJDDKr4mTc8-FX7--pd7j0vUbdk_1+KU0YniKEhRdee6SaS-8Q@mail.gmail.com","threadId":"32203","inReplyTo":"CAJELnLGq_oLBiNHANoaE7iEiA6g4fXX0PtJbqPFi4PQ+5LLvnA@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-11-27T06:20:20Z","receivedAt":"2012-11-27T06:20:20Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Nov 26, 2012 at 12:23 PM, Matt McClure\n wrote:\n> I'm finding the behavior of `git difftool -d` surprising. It seems that it\n> only uses symlinks to the working copy for files that are modified in the\n> working copy since the most recent commit. I would have expected it to use\n> symlinks for all files whose version under comparison is the working copy\n> version, regardless of whether the working copy differs from the HEAD.\n>\n> I'm using\n>\n> $ git --version\n> git version 1.8.0\n>\n> on a Mac from Homebrew.\n\ncc:ing Tim since he probably remembers this feature.\n\nThis is a side-effect of how it's currently implemented,\nand the general-purpose nature of the \"diff\" command.\n\ndiff can also be used for diffing arbitrary commits.\nThe simplest way to implement that is to create two temporary\ndirectories containing \"a/\" and \"b/\" and then launch the tool\nagainst them. That's what difftool does; it creates a temporary\nindex and uses `git checkout-index` to populate these two dirs.\n\nThe worktree handling is a bolt-on that symlinks\n(or copies (on windows or with --no-symlinks)) modified\nworktree files into one of these temporary directories.\n\nWhen symlinks are used (the default) we avoid needing to\ncopy these files back into the worktree; we can blindly\nremove the temporary directories without checking whether\nthe tool edited any files.\n\nWhen copies are used we check their content for changes\nbefore deciding to copy them back into the worktree.\n\nFiles that are not modified are not considered part of the\nset of files to check when copying back, or when symlinking,\nmostly because that's just how it's implemented right now.\n\nIt seems that there is an edge case here that we are not\naccounting for: unmodified worktree paths, when checked out\ninto the temporary directory, can be edited by the tool when\ncomparing against older commits. These edits will be lost.\n\nIf we had a way to know that either a/ or b/ can be replaced\nwith the worktree itself then we could make it even simpler.\n\nRight now we don't because difftool barely parses the\ncommand-line at all; most of it is parsed by git-diff.\nOriginally, difftool was a read-only tool so it was able to\navoid needing to know too much about what diff is really doing.\n\nWe would need to a way to re-use git's diff command-line parsing\nlogic to answer: \"is the worktree involved in this diff invocation?\"\n\nWhen we can do that then we avoid needing to have a temporary\ndirectory altogether for any dir-diffs that involve the worktree.\n\nDoes anyone know of a good way to answer that question?\n\nThe input is the command-line provided to diff/difftool.\nThe output is one of ('a', 'b', 'x'), where 'a' means the left\nside of the diff is the worktree, 'b' means the right side,\nand 'x' means neither (e.g. the command-line contains two refs).\n\nAssuming we can do this, it would also make dir-diff faster\nsince we can avoid needing to checkout the entire tree for\nthat side of the diff.\n-- \nDavid\n"},{"id":"204027","messageId":"CAJELnLHbNDCq1QecA9osxmKKNCaSghm9ADSt5tOrcdm=NF33og@mail.gmail.com","threadId":"32203","inReplyTo":"CAJDDKr4mTc8-FX7--pd7j0vUbdk_1+KU0YniKEhRdee6SaS-8Q@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2012-11-27T21:10:10Z","receivedAt":"2012-11-27T21:10:10Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tuesday, November 27, 2012, David Aguilar wrote:\n> It seems that there is an edge case here that we are not\n> accounting for: unmodified worktree paths, when checked out\n> into the temporary directory, can be edited by the tool when\n> comparing against older commits. These edits will be lost.\n\nYes. That is exactly my desired use case. I want to make edits while\nI'm reviewing the diff.\n\n>\n> When we can do that then we avoid needing to have a temporary\n> directory altogether for any dir-diffs that involve the worktree.\n\nI think the temporary directory is still a good thing. Without it, the\ndirectory diff tool would have no way to distinguish a file added in\nthe diff from a file that was preexisting and unmodified.\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211146","messageId":"CAJELnLGOK5m-JLwgfUdmQcS1exZMQdf1QR_g-GB_UhryDw3C9w@mail.gmail.com","threadId":"32203","inReplyTo":"CAJELnLEL8y0G3MBGkW+YDKtVxX4n4axJG7p0oPsXsV4_FRyGDg@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-12T18:12:29Z","receivedAt":"2013-03-12T18:12:29Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure wrote:\n>\n> On Tuesday, November 27, 2012, David Aguilar wrote:\n>>\n>> It seems that there is an edge case here that we are not\n>> accounting for: unmodified worktree paths, when checked out\n>> into the temporary directory, can be edited by the tool when\n>> comparing against older commits. These edits will be lost.\n>\n>\n> Yes. That is exactly my desired use case. I want to make edits while I'm reviewing the diff.\n\nI took a crack at implementing the change to make difftool -d use\nsymlinks more aggressively. I've tested it lightly, and it works for\nthe limited cases I've tried. This is my first foray into the Git\nsource code, so it's entirely possible that there are unintended side\neffects and regressions if other features depend on the same code path\nand make different assumptions.\n\nhttps://github.com/matthewlmcclure/git/compare/difftool-directory-symlink-work-tree\n\nYour thoughts on the change?\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211148","messageId":"20130312190956.GC2317@serenity.lan","threadId":"32203","inReplyTo":"CAJELnLGOK5m-JLwgfUdmQcS1exZMQdf1QR_g-GB_UhryDw3C9w@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-12T19:09:56Z","receivedAt":"2013-03-12T19:09:56Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:\n> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure wrote:\n> Your thoughts on the change?\n\nPlease include the patch in your message so that interested parties can\ncomment on it here, especially since the compare view on GitHub seems to\nmangle the tabs.\n\nFor others' reference the patch is:\n\n-- >8 --\nFrom: Matt McClure \nSubject: [PATCH] difftool: Make directory diff symlink work tree\n\ndifftool -d formerly knew how to symlink to the work tree when the work\ntree contains uncommitted changes. In practice, prior to this change, it\nwould not symlink to the work tree in case there were no uncommitted\nchanges, even when the user invoked difftool with the form:\n\n git difftool -d [--options] [--] [...]\n This form is to view the changes you have in your working tree\n relative to the named . You can use HEAD to compare it\n with the latest commit, or a branch name to compare with the tip\n of a different branch.\n\nInstead, prior to this change, difftool would use the file's HEAD blob\nsha1 to find its content rather than the work tree content. This change\nteaches `git diff --raw` to emit the null SHA1 for consumption by\ndifftool -d, so that difftool -d will use a symlink rather than a copy\nof the file.\n\nBefore:\n\n $ git diff --raw HEAD^ -- diff-lib.c\n :100644 100644 f35de0f... ead9399... M diff-lib.c\n\nAfter:\n\n $ ./git diff --raw HEAD^ -- diff-lib.c\n :100644 100644 f35de0f... 0000000... M diff-lib.c\n---\n diff-lib.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f35de0f..ead9399 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,\n \t\treturn -1;\n \t}\n \n+\tif (!cached && hashcmp(old->sha1, new->sha1)) {\n+\t\tsha1 = null_sha1;\n+\t}\n+\n \tif (revs->combine_merges && !cached &&\n \t (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {\n \t\tstruct combine_diff_path *p;\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211150","messageId":"CAJDDKr7S0ex1RvZS0QeBXxAuqcKrQJzhZeJP0MoMGmpGXyMOrA@mail.gmail.com","threadId":"32203","inReplyTo":"20130312190956.GC2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-12T19:23:52Z","receivedAt":"2013-03-12T19:23:52Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Mar 12, 2013 at 12:09 PM, John Keeping wrote:\n> On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:\n>> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure wrote:\n>> Your thoughts on the change?\n>\n> Please include the patch in your message so that interested parties can\n> comment on it here, especially since the compare view on GitHub seems to\n> mangle the tabs.\n>\n> For others' reference the patch is:\n>\n> -- >8 --\n> From: Matt McClure \n> Subject: [PATCH] difftool: Make directory diff symlink work tree\n>\n> difftool -d formerly knew how to symlink to the work tree when the work\n> tree contains uncommitted changes. In practice, prior to this change, it\n> would not symlink to the work tree in case there were no uncommitted\n> changes, even when the user invoked difftool with the form:\n>\n> git difftool -d [--options] [--] [...]\n> This form is to view the changes you have in your working tree\n> relative to the named . You can use HEAD to compare it\n> with the latest commit, or a branch name to compare with the tip\n> of a different branch.\n>\n> Instead, prior to this change, difftool would use the file's HEAD blob\n> sha1 to find its content rather than the work tree content. This change\n> teaches `git diff --raw` to emit the null SHA1 for consumption by\n> difftool -d, so that difftool -d will use a symlink rather than a copy\n> of the file.\n>\n> Before:\n>\n> $ git diff --raw HEAD^ -- diff-lib.c\n> :100644 100644 f35de0f... ead9399... M diff-lib.c\n>\n> After:\n>\n> $ ./git diff --raw HEAD^ -- diff-lib.c\n> :100644 100644 f35de0f... 0000000... M diff-lib.c\n\n\nInteresting approach. While this does get the intended behavior\nfor difftool, I'm afraid this would be a grave regression for\nexisting \"git diff --raw\" users who cannot have such behavior.\n\nI don't think we could do this without adding an additional flag\nto trigger this change in behavior (e.g. --null-sha1-for-....?)\nso that existing users are unaffected by the change.\n\nIt feels like forcing the null SHA-1 is heavy-handed, but I\nhaven't thought it through enough.\n\nWhile this may be a quick way to get this behavior,\nI wonder if there is a better way.\n\nDoes anybody else have any comments/suggestions on how to\nbetter accomplish this?\n\n\n> ---\n> diff-lib.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index f35de0f..ead9399 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,\n> return -1;\n> }\n>\n> + if (!cached && hashcmp(old->sha1, new->sha1)) {\n> + sha1 = null_sha1;\n> + }\n> +\n> if (revs->combine_merges && !cached &&\n> (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {\n> struct combine_diff_path *p;\n> --\n> 1.8.2.rc2.4.g7799588\n>\n\n\n\n-- \nDavid\n"},{"id":"211151","messageId":"20130312192459.GD2317@serenity.lan","threadId":"32203","inReplyTo":"20130312190956.GC2317@serenity.lan","subject":"Re: [PATCH] difftool: Make directory diff symlink work tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-12T19:24:59Z","receivedAt":"2013-03-12T19:24:59Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"> difftool -d formerly knew how to symlink to the work tree when the work\n> tree contains uncommitted changes. In practice, prior to this change, it\n> would not symlink to the work tree in case there were no uncommitted\n> changes, even when the user invoked difftool with the form:\n> \n> git difftool -d [--options] [--] [...]\n> This form is to view the changes you have in your working tree\n> relative to the named . You can use HEAD to compare it\n> with the latest commit, or a branch name to compare with the tip\n> of a different branch.\n> \n> Instead, prior to this change, difftool would use the file's HEAD blob\n> sha1 to find its content rather than the work tree content. This change\n> teaches `git diff --raw` to emit the null SHA1 for consumption by\n> difftool -d, so that difftool -d will use a symlink rather than a copy\n> of the file.\n> \n> Before:\n> \n> $ git diff --raw HEAD^ -- diff-lib.c\n> :100644 100644 f35de0f... ead9399... M diff-lib.c\n> \n> After:\n> \n> $ ./git diff --raw HEAD^ -- diff-lib.c\n> :100644 100644 f35de0f... 0000000... M diff-lib.c\n\nWhen I tried this I got the expected behaviour even without this patch.\n\nIt turns out that an uncommitted, but *staged* change emits the SHA1 of\nthe blob rather than the null SHA1. Do you get the desired behaviour if\nyou \"git reset\" before using difftool?\n\nIf so I think you want some new mode of operation for difftool instead\nof this patch which will also affect unrelated commands.\n\n> ---\n> diff-lib.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index f35de0f..ead9399 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,\n> \t\treturn -1;\n> \t}\n> \n> +\tif (!cached && hashcmp(old->sha1, new->sha1)) {\n> +\t\tsha1 = null_sha1;\n> +\t}\n> +\n> \tif (revs->combine_merges && !cached &&\n> \t (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {\n> \t\tstruct combine_diff_path *p;\n> -- \n> 1.8.2.rc2.4.g7799588\n> \n"},{"id":"211155","messageId":"20130312194306.GE2317@serenity.lan","threadId":"32203","inReplyTo":"CAJDDKr7S0ex1RvZS0QeBXxAuqcKrQJzhZeJP0MoMGmpGXyMOrA@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-12T19:43:06Z","receivedAt":"2013-03-12T19:43:06Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 12:23:52PM -0700, David Aguilar wrote:\n> I don't think we could do this without adding an additional flag\n> to trigger this change in behavior (e.g. --null-sha1-for-....?)\n> so that existing users are unaffected by the change.\n> \n> It feels like forcing the null SHA-1 is heavy-handed, but I\n> haven't thought it through enough.\n> \n> While this may be a quick way to get this behavior,\n> I wonder if there is a better way.\n> \n> Does anybody else have any comments/suggestions on how to\n> better accomplish this?\n\nHow about something like \"--symlink-all\" where the everything in the\nright-hand tree is symlink'd?\n\nSomething like this perhaps:\n\n-- >8 --\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 0a90de4..cab7c45 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -85,7 +85,7 @@ sub exit_cleanup\n \n sub setup_dir_diff\n {\n-\tmy ($repo, $workdir, $symlinks) = @_;\n+\tmy ($repo, $workdir, $symlinks, $symlink_all) = @_;\n \n \t# Run the diff; exit immediately if no diff found\n \t# 'Repository' and 'WorkingCopy' must be explicitly set to insure that\n@@ -159,10 +159,10 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\tif ($rsha1 ne $null_sha1) {\n-\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n-\t\t\t} else {\n+\t\t\tif ($symlink_all or $rsha1 eq $null_sha1) {\n \t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\t} else {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n \t\t}\n \t}\n@@ -299,6 +299,7 @@ sub main\n \t\tprompt => undef,\n \t\tsymlinks => $^O ne 'cygwin' &&\n \t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n+\t\tsymlink_all => undef,\n \t\ttool_help => undef,\n \t);\n \tGetOptions('g|gui!' => \\$opts{gui},\n@@ -308,6 +309,7 @@ sub main\n \t\t'y' => sub { $opts{prompt} = 0; },\n \t\t'symlinks' => \\$opts{symlinks},\n \t\t'no-symlinks' => sub { $opts{symlinks} = 0; },\n+\t\t'symlink-all' => \\$opts{symlink_all},\n \t\t't|tool:s' => \\$opts{difftool_cmd},\n \t\t'tool-help' => \\$opts{tool_help},\n \t\t'x|extcmd:s' => \\$opts{extcmd});\n@@ -346,7 +348,7 @@ sub main\n \t# will invoke a separate instance of 'git-difftool--helper' for\n \t# each file that changed.\n \tif (defined($opts{dirdiff})) {\n-\t\tdir_diff($opts{extcmd}, $opts{symlinks});\n+\t\tdir_diff($opts{extcmd}, $opts{symlinks}, $opts{symlink_all});\n \t} else {\n \t\tfile_diff($opts{prompt});\n \t}\n@@ -354,13 +356,13 @@ sub main\n \n sub dir_diff\n {\n-\tmy ($extcmd, $symlinks) = @_;\n+\tmy ($extcmd, $symlinks, $symlink_all) = @_;\n \tmy $rc;\n \tmy $error = 0;\n \tmy $repo = Git->repository();\n \tmy $workdir = find_worktree($repo);\n \tmy ($a, $b, $tmpdir, @worktree) =\n-\t\tsetup_dir_diff($repo, $workdir, $symlinks);\n+\t\tsetup_dir_diff($repo, $workdir, $symlinks, $symlink_all);\n \n \tif (defined($extcmd)) {\n \t\t$rc = system($extcmd, $a, $b);\n"},{"id":"211158","messageId":"7vk3pc73r1.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"CAJDDKr7S0ex1RvZS0QeBXxAuqcKrQJzhZeJP0MoMGmpGXyMOrA@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-12T20:38:26Z","receivedAt":"2013-03-12T20:38:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Interesting approach. While this does get the intended behavior\n> for difftool, I'm afraid this would be a grave regression for\n> existing \"git diff --raw\" users who cannot have such behavior.\n\nThe 0{40} in RHS of --raw output is to say that we do not know what\nobject name the contents at the path hashes to.\n\nIf you run \"git diff HEAD^\" for a path that is different between\nHEAD and the index for which you do not have a local change in the\nworking tree, we have to show the path (because it is different\nbetween the working tree and HEAD^), but we know the object name for\ncopy in the working tree, simply because we know it matches what is\nin the index. Showing 0{40} on the RHS in such a case loses\ninformation, making us say \"We don't know\" when we perfectly well\nknow. That is a regression.\n\nIf the user is allowed to touch any random file in the working tree,\nI do not see a workable solution other than John Keeping's follow-up\npatch to make symlinks of all paths involved.\n"},{"id":"211159","messageId":"7vfw0073pm.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130312194306.GE2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-12T20:39:17Z","receivedAt":"2013-03-12T20:39:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> How about something like \"--symlink-all\" where the everything in the\n> right-hand tree is symlink'd?\n\nDoes it even have to be conditional? What is the situation when you\ndo not want symbolic links?\n"},{"id":"211161","messageId":"20130312210630.GF2317@serenity.lan","threadId":"32203","inReplyTo":"7vfw0073pm.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-12T21:06:30Z","receivedAt":"2013-03-12T21:06:30Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> \n> > How about something like \"--symlink-all\" where the everything in the\n> > right-hand tree is symlink'd?\n> \n> Does it even have to be conditional? What is the situation when you\n> do not want symbolic links?\n\nWhen you're not comparing the working tree.\n\nIf we can reliably say \"the RHS is the working tree\" then it could be\nunconditional, but I haven't thought about how to do that - I can't see\na particularly easy way to check for that; is it sufficient to say\n\"there is no more than one non-option to the left of '--' and '--cached'\nis not among the options\"?\n\n\nJohn\n"},{"id":"211163","messageId":"7vy5ds5mz0.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130312210630.GF2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-12T21:26:11Z","receivedAt":"2013-03-12T21:26:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n>> Does it even have to be conditional? What is the situation when you\n>> do not want symbolic links?\n>\n> When you're not comparing the working tree.\n\nOK, so what you want is essentially:\n\n * If you see 0{40} in \"diff --raw\", you *know* you are showing the working tree\n file on the RHS, and you want to symlink, so that the edit made\n by the user will be reflected back to theh working tree copy.\n \n * If your working tree file match what is in the index, you won't\n see 0{40} but you still want to symlink, for the same reason.\n\n * If you are comparing two trees, and especially if your RHS is not\n HEAD, you will send everything to a temporary without\n symlinks. Any edit made by the user will be lost.\n\nIf that is the case, perhaps the safest way to go may be to write\nthe object out when you see non 0{40}, compare it with the working\ntree version and then turn that into symlink? That way, you not\nonly cover the second bullet point, but also cover half of the third\none where the user may find a bug in the RHS, update it in difftool.\n\nI am assuming that you are write-protecting the non-symlink files in\nthe temporary tree (i.e. those that do not match the working tree)\nto prevent users from accidentally modifying something there is no\nplace to save back to.\n\nHrm?\n"},{"id":"211166","messageId":"CAJELnLGBr1wOX4-3rCNjPpPLezc_6FgyeuPqty268JR0==qtvQ@mail.gmail.com","threadId":"32203","inReplyTo":"20130312210630.GF2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-12T21:43:37Z","receivedAt":"2013-03-12T21:43:37Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tue, Mar 12, 2013 at 5:06 PM, John Keeping wrote:\n> On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:\n>>\n>> What is the situation when you do not want symbolic links?\n>\n> When you're not comparing the working tree.\n>\n> If we can reliably say \"the RHS is the working tree\" then it could be\n> unconditional, but I haven't thought about how to do that - I can't see\n> a particularly easy way to check for that;\n\nAgreed. From what I can see, the only form of the diff options that\ncompares against the working tree is\n\n git difftool -d [--options] [--] [...]\n\nAt first, I thought that the following cases were also working tree\ncases, but actually they use the HEAD commit.\n\n git difftool -d commit1..\n git difftool -d commit1...\n\n> is it sufficient to say\n> \"there is no more than one non-option to the left of '--' and '--cached'\n> is not among the options\"?\n\nAn alternative approach would be to reuse git-diff's option parsing\nand make it tell git-difftool when git-diff sees the working tree\ncase. At this point, I haven't seen an obvious place in the source\nwhere git-diff makes that choice, but if someone could point me in the\nright direction, I think I'd actually prefer that approach. What do\nyou think?\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211170","messageId":"CAJELnLGenaFR1zeq=+2Ed6CCbog7q9aFm=B4PN2poJVhGxLBww@mail.gmail.com","threadId":"32203","inReplyTo":"CAJELnLGBr1wOX4-3rCNjPpPLezc_6FgyeuPqty268JR0==qtvQ@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-12T22:11:14Z","receivedAt":"2013-03-12T22:11:14Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tue, Mar 12, 2013 at 5:43 PM, Matt McClure wrote:\n> On Tue, Mar 12, 2013 at 5:06 PM, John Keeping wrote:\n>>\n>> is it sufficient to say\n>> \"there is no more than one non-option to the left of '--' and '--cached'\n>> is not among the options\"?\n>\n> An alternative approach would be to reuse git-diff's option parsing\n> and make it tell git-difftool when git-diff sees the working tree\n> case. At this point, I haven't seen an obvious place in the source\n> where git-diff makes that choice, but if someone could point me in the\n> right direction, I think I'd actually prefer that approach. What do\n> you think?\n\nThere's an interesting comment in cmd_diff:\n\n/*\n* We could get N tree-ish in the rev.pending_objects list.\n* Also there could be M blobs there, and P pathspecs.\n*\n* N=0, M=0:\n* cache vs files (diff-files)\n* N=0, M=2:\n* compare two random blobs. P must be zero.\n* N=0, M=1, P=1:\n* compare a blob with a working tree file.\n*\n* N=1, M=0:\n* tree vs cache (diff-index --cached)\n*\n* N=2, M=0:\n* tree vs tree (diff-tree)\n*\n* N=0, M=0, P=2:\n* compare two filesystem entities (aka --no-index).\n*\n* Other cases are errors.\n*/\n\nwhereas inspecting rev.pending in the \"compare against working tree\"\ncase, I see:\n\n(gdb) p rev.pending\n$3 = {\n nr = 1,\n alloc = 64,\n objects = 0x100807a00\n}\n(gdb) p *rev.pending.objects\n$4 = {\n item = 0x100831a48,\n name = 0x7fff5fbff8f8 \"HEAD^\",\n mode = 12288\n}\n\nGiven the cases listed in the comment, I assume cmd_diff must\ninterpret this case as:\n\n* N=1, M=0:\n* tree vs cache (diff-index --cached)\n\nThe description of that case is confusing or wrong given that\ngit-diff-index(1) says:\n\n --cached\n do not consider the on-disk file at all\n\n***\n\ncmd_diff executes this case:\n\nelse if (ents == 1)\n result = builtin_diff_index(&rev, argc, argv);\n\nSo it looks like I could short-circuit in builtin_diff_index or\nsomething it calls -- e.g., run_diff_index -- to get git-diff to tell\ngit-difftool that it's the working tree case. I see that\nrun_diff_index does:\n\n diff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\n\nSo that looks like a good place where the code is already deciding\nthat it's the working tree case -- \"w/\", though surprisingly to me:\n\n(gdb) p revs->diffopt\n$12 = {\n...\n a_prefix = 0x1001c25aa \"a/\",\n b_prefix = 0x1001c25ad \"b/\",\n...\n\nSo diff_set_mnemonic_prefix doesn't actually use the \"w/\" value passed\nto it because:\n\nif (!options->b_prefix)\n options->b_prefix = b;\n\nMaybe if I could prevent b_prefix from getting set earlier, I could\nget some variant of git-diff to emit the \"w/\" for git-difftool.\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211171","messageId":"7vehfk5kn2.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"CAJELnLGBr1wOX4-3rCNjPpPLezc_6FgyeuPqty268JR0==qtvQ@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-12T22:16:33Z","receivedAt":"2013-03-12T22:16:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McClure writes:\n\n> An alternative approach would be to reuse git-diff's option parsing\n> and make it tell git-difftool when git-diff sees the working tree\n> case. At this point, I haven't seen an obvious place in the source\n> where git-diff makes that choice, but if someone could point me in the\n> right direction, I think I'd actually prefer that approach. What do\n> you think?\n\nI do not think you want to go there. That wouldn't solve the third\ncase in my previous message, no?\n"},{"id":"211173","messageId":"3222724986386016520@unknownmsgid","threadId":"32203","inReplyTo":"7vehfk5kn2.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-12T22:48:16Z","receivedAt":"2013-03-12T22:48:16Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Mar 12, 2013, at 4:16 PM, Junio C Hamano wrote:\n\n> Matt McClure writes:\n>\n>> An alternative approach would be to reuse git-diff's option parsing\n>\n> I do not think you want to go there. That wouldn't solve the third\n> case in my previous message, no?\n\nI think I don't fully understand your third bullet.\n\n> * If you are comparing two trees, and especially if your RHS is not\n> HEAD, you will send everything to a temporary without\n> symlinks. Any edit made by the user will be lost.\n\nI think you're suggesting to use a symlink any time the content of any\ngiven RHS revision is the same as the working tree.\n\nI imagine that might confuse me as a user. It would create\ncircumstances where some files are symlinked and others aren't for\nreasons that won't be straightforward.\n\nI imagine solving that case, I might instead implement a copy back to\nthe working tree with conflict detection/resolution. Some earlier\niterations of the directory diff feature used copy back without\nconflict detection and created situations where I clobbered my own\nchanges by finishing a directory diff after making edits concurrently.\n"},{"id":"211174","messageId":"7906294865355046191@unknownmsgid","threadId":"32203","inReplyTo":"20130312192459.GD2317@serenity.lan","subject":"Re: [PATCH] difftool: Make directory diff symlink work tree","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-12T23:12:28Z","receivedAt":"2013-03-12T23:12:28Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Mar 12, 2013, at 1:25 PM, John Keeping wrote:\n\n> When I tried this I got the expected behaviour even without this patch.\n\n git diff --raw commit\n\nemits the null SHA1 if the working tree file's stat differs from the\nblob corresponding to commit. Is that the case you observed?\n"},{"id":"211178","messageId":"20130312234050.GG2317@serenity.lan","threadId":"32203","inReplyTo":"7906294865355046191@unknownmsgid","subject":"Re: [PATCH] difftool: Make directory diff symlink work tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-12T23:40:50Z","receivedAt":"2013-03-12T23:40:50Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 05:12:28PM -0600, Matt McClure wrote:\n> On Mar 12, 2013, at 1:25 PM, John Keeping wrote:\n> \n> > When I tried this I got the expected behaviour even without this patch.\n> \n> git diff --raw commit\n> \n> emits the null SHA1 if the working tree file's stat differs from the\n> blob corresponding to commit. Is that the case you observed?\n\nYes, although it's slightly more subtle than that - the null SHA1 only\noccurs if the working tree file has unstaged changes; if you add the\nchanges to the index then the null SHA1 is no longer used since the blob\nis now available in Git's object store.\n\n\nJohn\n"},{"id":"211181","messageId":"20130313001758.GH2317@serenity.lan","threadId":"32203","inReplyTo":"3222724986386016520@unknownmsgid","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T00:17:59Z","receivedAt":"2013-03-13T00:17:59Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:\n> On Mar 12, 2013, at 4:16 PM, Junio C Hamano wrote:\n> \n> > Matt McClure writes:\n> >\n> > * If you are comparing two trees, and especially if your RHS is not\n> > HEAD, you will send everything to a temporary without\n> > symlinks. Any edit made by the user will be lost.\n> \n> I think you're suggesting to use a symlink any time the content of any\n> given RHS revision is the same as the working tree.\n> \n> I imagine that might confuse me as a user. It would create\n> circumstances where some files are symlinked and others aren't for\n> reasons that won't be straightforward.\n> \n> I imagine solving that case, I might instead implement a copy back to\n> the working tree with conflict detection/resolution. Some earlier\n> iterations of the directory diff feature used copy back without\n> conflict detection and created situations where I clobbered my own\n> changes by finishing a directory diff after making edits concurrently.\n\nThe code to copy back working tree files is already there, it just\ntriggers using the same logic as the creation of symlinks in the first\nplace and doesn't attempt any conflict detection. I suspect that any\nmore comprehensive solution will need to restrict the use of \"git\ndifftool -d\" whenever the index contains unmerged entries or when there\nare both staged and unstaged changes, since the merge resolution will\ncause these states to be lost.\n\nThe implementation of Junio's suggestion is relatively straightforward\n(this is untested, although t7800 passes, and can probably be improved\nby someone better versed in Perl). Does this work for your original\nscenario?\n\n-- >8 --\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 0a90de4..5f093ae 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -83,6 +83,21 @@ sub exit_cleanup\n \texit($status | ($status >> 8));\n }\n \n+sub use_wt_file\n+{\n+\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy $null_sha1 = '0' x 40;\n+\n+\tif ($sha1 eq $null_sha1) {\n+\t\treturn 1;\n+\t} elsif (not $symlinks) {\n+\t\treturn 0;\n+\t}\n+\n+\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n+\treturn $sha1 eq $wt_sha1;\n+}\n+\n sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n@@ -159,10 +174,10 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\tif ($rsha1 ne $null_sha1) {\n-\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n-\t\t\t} else {\n+\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n \t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\t} else {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n \t\t}\n \t}\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211182","messageId":"CAJELnLEmrBSiua3xe_Y7MS1SCL8TD28sQH-R6Kfn9Zk+Zm6=kw@mail.gmail.com","threadId":"32203","inReplyTo":"20130312192459.GD2317@serenity.lan","subject":"Re: [PATCH] difftool: Make directory diff symlink work tree","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-13T00:26:21Z","receivedAt":"2013-03-13T00:26:21Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tue, Mar 12, 2013 at 3:24 PM, John Keeping wrote:\n> When I tried this I got the expected behaviour even without this patch.\n>\n> It turns out that an uncommitted, but *staged* change emits the SHA1 of\n> the blob rather than the null SHA1. Do you get the desired behaviour if\n> you \"git reset\" before using difftool?\n\nI tried this:\n\n$ git diff --raw HEAD^\n:100644 100644 f35de0f... ead9399... M diff-lib.c\n\n$ git reset HEAD^\nUnstaged changes after reset:\nM diff-lib.c\n\n$ git diff --raw\n:100644 100644 f35de0f... 0000000... M diff-lib.c\n\n$ git difftool -d\n\nand the last command did indeed create symlinks into my working tree\nrather than file copies.\n\nSo... it seems that using git-reset is at least a workaround to get\nthe symlink behavior I want as a user, though the dance I have to do\nis a little more awkward than `git difftool -d HEAD^` would be.\n\n> If so I think you want some new mode of operation for difftool instead\n> of this patch which will also affect unrelated commands.\n\nAre you suggesting that difftool do the reset work above given a new\noption or by default?\n\n--\nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211183","messageId":"CAJELnLHGNit90LWMtqY_oFZPScKA2+xx4+2MfJxoZs3kYD1G6w@mail.gmail.com","threadId":"32203","inReplyTo":"20130313001758.GH2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-13T00:56:11Z","receivedAt":"2013-03-13T00:56:11Z","isPatch":false,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Tue, Mar 12, 2013 at 8:17 PM, John Keeping wrote:\n> Does this work for your original scenario?\n\nYes. Thanks!\n\n-- \nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"211200","messageId":"CAJDDKr7ZU16XWtCfYX9-RMzcpKa_FF80Od+mUMG4n8dUKeLsvw@mail.gmail.com","threadId":"32203","inReplyTo":"20130313001758.GH2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-13T08:24:07Z","receivedAt":"2013-03-13T08:24:07Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Mar 12, 2013 at 5:17 PM, John Keeping wrote:\n> On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:\n>> On Mar 12, 2013, at 4:16 PM, Junio C Hamano wrote:\n>>\n>> > Matt McClure writes:\n>> >\n>> > * If you are comparing two trees, and especially if your RHS is not\n>> > HEAD, you will send everything to a temporary without\n>> > symlinks. Any edit made by the user will be lost.\n>>\n>> I think you're suggesting to use a symlink any time the content of any\n>> given RHS revision is the same as the working tree.\n>>\n>> I imagine that might confuse me as a user. It would create\n>> circumstances where some files are symlinked and others aren't for\n>> reasons that won't be straightforward.\n>>\n>> I imagine solving that case, I might instead implement a copy back to\n>> the working tree with conflict detection/resolution. Some earlier\n>> iterations of the directory diff feature used copy back without\n>> conflict detection and created situations where I clobbered my own\n>> changes by finishing a directory diff after making edits concurrently.\n>\n> The code to copy back working tree files is already there, it just\n> triggers using the same logic as the creation of symlinks in the first\n> place and doesn't attempt any conflict detection. I suspect that any\n> more comprehensive solution will need to restrict the use of \"git\n> difftool -d\" whenever the index contains unmerged entries or when there\n> are both staged and unstaged changes, since the merge resolution will\n> cause these states to be lost.\n>\n> The implementation of Junio's suggestion is relatively straightforward\n> (this is untested, although t7800 passes, and can probably be improved\n> by someone better versed in Perl). Does this work for your original\n> scenario?\n\nThis is a nice straightforward approach.\n\nAs Junio mentioned, a good next step would be this patch\nin combination with making the truly temporary files\ncreated by dir-diff readonly.\n\nWill that need a win32 platform check?\nDoes anyone want to take this and whip it into a proper patch?\n\n> -- >8 --\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 0a90de4..5f093ae 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -83,6 +83,21 @@ sub exit_cleanup\n> exit($status | ($status >> 8));\n> }\n>\n> +sub use_wt_file\n> +{\n> + my ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> + my $null_sha1 = '0' x 40;\n> +\n> + if ($sha1 eq $null_sha1) {\n> + return 1;\n> + } elsif (not $symlinks) {\n> + return 0;\n> + }\n> +\n> + my $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> + return $sha1 eq $wt_sha1;\n> +}\n> +\n> sub setup_dir_diff\n> {\n> my ($repo, $workdir, $symlinks) = @_;\n> @@ -159,10 +174,10 @@ EOF\n> }\n>\n> if ($rmode ne $null_mode) {\n> - if ($rsha1 ne $null_sha1) {\n> - $rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> - } else {\n> + if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> push(@working_tree, $dst_path);\n> + } else {\n> + $rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> }\n> }\n> }\n> --\n> 1.8.2.rc2.4.g7799588\n>\n-- \nDavid\n"},{"id":"211204","messageId":"20130313091134.GI2317@serenity.lan","threadId":"32203","inReplyTo":"CAJELnLEmrBSiua3xe_Y7MS1SCL8TD28sQH-R6Kfn9Zk+Zm6=kw@mail.gmail.com","subject":"Re: [PATCH] difftool: Make directory diff symlink work tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T09:11:34Z","receivedAt":"2013-03-13T09:11:34Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 12, 2013 at 08:26:21PM -0400, Matt McClure wrote:\n> On Tue, Mar 12, 2013 at 3:24 PM, John Keeping wrote:\n> > If so I think you want some new mode of operation for difftool instead\n> > of this patch which will also affect unrelated commands.\n> \n> Are you suggesting that difftool do the reset work above given a new\n> option or by default?\n\nI was suggesting something like the \"--symlink-all\" option discussed in\nthe parallel thread, but it looks like we now have a better solution\nthan that.\n\n\nJohn\n"},{"id":"211218","messageId":"7vtxof48sg.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"CAJDDKr7ZU16XWtCfYX9-RMzcpKa_FF80Od+mUMG4n8dUKeLsvw@mail.gmail.com","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-13T15:30:07Z","receivedAt":"2013-03-13T15:30:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n>> The implementation of Junio's suggestion is relatively straightforward\n>> (this is untested, although t7800 passes, and can probably be improved\n>> by someone better versed in Perl). Does this work for your original\n>> scenario?\n>\n> This is a nice straightforward approach.\n>\n> As Junio mentioned, a good next step would be this patch in\n> combination with making the truly temporary files created by\n> dir-diff readonly.\n\nEven though I agree that the idea Matt McClure mentioned to run a\nthree-way merge to take the modification back when the path checked\nout to the temporary tree as a temporary file (because it does not\nmatch the working tree version) gets edited by the user might be a\nbetter longer-term direction to go, marking the temporaries that the\nusers should not modify clearly as such needs to be done in the\nshorter term. This thread wouldn't have had to happen if we had\nsuch a safety measure in the first place.\n"},{"id":"211229","messageId":"7v1ubj45ac.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"7vtxof48sg.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-13T16:45:47Z","receivedAt":"2013-03-13T16:45:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> David Aguilar writes:\n>\n>>> The implementation of Junio's suggestion is relatively straightforward\n>>> (this is untested, although t7800 passes, and can probably be improved\n>>> by someone better versed in Perl). Does this work for your original\n>>> scenario?\n>>\n>> This is a nice straightforward approach.\n>>\n>> As Junio mentioned, a good next step would be this patch in\n>> combination with making the truly temporary files created by\n>> dir-diff readonly.\n>\n> Even though I agree that the idea Matt McClure mentioned to run a\n> three-way merge to take the modification back when the path checked\n> out to the temporary tree as a temporary file (because it does not\n> match the working tree version) gets edited by the user might be a\n> better longer-term direction to go, marking the temporaries that the\n> users should not modify clearly as such needs to be done in the\n> shorter term. This thread wouldn't have had to happen if we had\n> such a safety measure in the first place.\n\nOne thing I forgot to add. I suspect the patch in the thread will\nnot work if the path needs smudge filter and end-of-line conversion,\nas it seems to just hash-object what is in the working tree (which\nshould be _after_ these transformations) and compare with the object\nname. The comparison should go the other way around. Try to check\nout the object with these transformations applied, and compare the\nresulting file with what is in the working tree.\n\nDoes the temporary checkout correctly apply the smudge filter and\ncrlf conversion, by the way? If not, regardless of the topic in\nthis thread, that may want to be fixed as well. I didn't check.\n"},{"id":"211231","messageId":"20130313170821.GK2317@serenity.lan","threadId":"32203","inReplyTo":"7v1ubj45ac.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T17:08:21Z","receivedAt":"2013-03-13T17:08:21Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > David Aguilar writes:\n> >\n> >>> The implementation of Junio's suggestion is relatively straightforward\n> >>> (this is untested, although t7800 passes, and can probably be improved\n> >>> by someone better versed in Perl). Does this work for your original\n> >>> scenario?\n> >>\n> >> This is a nice straightforward approach.\n> >>\n> >> As Junio mentioned, a good next step would be this patch in\n> >> combination with making the truly temporary files created by\n> >> dir-diff readonly.\n> >\n> > Even though I agree that the idea Matt McClure mentioned to run a\n> > three-way merge to take the modification back when the path checked\n> > out to the temporary tree as a temporary file (because it does not\n> > match the working tree version) gets edited by the user might be a\n> > better longer-term direction to go, marking the temporaries that the\n> > users should not modify clearly as such needs to be done in the\n> > shorter term. This thread wouldn't have had to happen if we had\n> > such a safety measure in the first place.\n> \n> One thing I forgot to add. I suspect the patch in the thread will\n> not work if the path needs smudge filter and end-of-line conversion,\n> as it seems to just hash-object what is in the working tree (which\n> should be _after_ these transformations) and compare with the object\n> name. The comparison should go the other way around. Try to check\n> out the object with these transformations applied, and compare the\n> resulting file with what is in the working tree.\n\ngit-hash-object(1) implies that it will apply the clean filter and EOL\nconversion when it's given a path to a file in the working tree (as it\nis here). Is that not the case?\n\n\nJohn\n"},{"id":"211233","messageId":"7vppz32o60.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130313170821.GK2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-13T17:40:55Z","receivedAt":"2013-03-13T17:40:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> git-hash-object(1) implies that it will apply the clean filter and EOL\n> conversion when it's given a path to a file in the working tree (as it\n> is here). Is that not the case?\n\nApplying clean to smudged contents _ought to_ recover clean version,\nbut is that \"ought to\" something you would want to rely on?\n"},{"id":"211240","messageId":"20130313180106.GL2317@serenity.lan","threadId":"32203","inReplyTo":"7vppz32o60.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T18:01:06Z","receivedAt":"2013-03-13T18:01:06Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> \n> > git-hash-object(1) implies that it will apply the clean filter and EOL\n> > conversion when it's given a path to a file in the working tree (as it\n> > is here). Is that not the case?\n> \n> Applying clean to smudged contents _ought to_ recover clean version,\n> but is that \"ought to\" something you would want to rely on?\n\nHow does git-status figure out that file that has been touch'd does not\nhave unstaged changes without relying on this? Surely this case is no\ndifferent from that?\n\n\nJohn\n"},{"id":"211243","messageId":"7vy5dr14mc.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130313180106.GL2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-13T19:28:27Z","receivedAt":"2013-03-13T19:28:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:\n>> John Keeping writes:\n>> \n>> > git-hash-object(1) implies that it will apply the clean filter and EOL\n>> > conversion when it's given a path to a file in the working tree (as it\n>> > is here). Is that not the case?\n>> \n>> Applying clean to smudged contents _ought to_ recover clean version,\n>> but is that \"ought to\" something you would want to rely on?\n>\n> How does git-status figure out that file that has been touch'd does not\n> have unstaged changes without relying on this? Surely this case is no\n> different from that?\n\nI just checked. ce_modified_check_fs() does ce_compare_data() which\ndoes the same \"hash the path and compare the resulting hash\". So I\nthink we are OK.\n\nThanks.\n"},{"id":"211249","messageId":"cover.1363206651.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"7vy5dr14mc.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T20:33:07Z","receivedAt":"2013-03-13T20:33:07Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Here's the proper patch. It grew into a series because I noticed a\nminor formatting error in the difftool documentation, which the first\ncommit fixes.\n\nThe content of the second patch is the same as was previously posted.\n\nJohn Keeping (2):\n git-difftool(1): fix formatting of --symlink description\n difftool --dir-diff: symlink all files matching the working tree\n\n Documentation/git-difftool.txt | 8 +++++---\n git-difftool.perl | 21 ++++++++++++++++++---\n t/t7800-difftool.sh | 14 ++++++++++++++\n 3 files changed, 37 insertions(+), 6 deletions(-)\n\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211250","messageId":"926d8f9458ffcce9c3883c2b4ec7a220e268eba2.1363206651.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363206651.git.john@keeping.me.uk","subject":"[PATCH 1/2] git-difftool(1): fix formatting of --symlink description","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T20:33:08Z","receivedAt":"2013-03-13T20:33:08Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Signed-off-by: John Keeping \n---\n Documentation/git-difftool.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex e0e12e9..e575fea 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -74,8 +74,8 @@ with custom merge tool commands and has the same value as `$MERGED`.\n \t'git difftool''s default behavior is create symlinks to the\n \tworking tree when run in `--dir-diff` mode.\n +\n-\tSpecifying `--no-symlinks` instructs 'git difftool' to create\n-\tcopies instead. `--no-symlinks` is the default on Windows.\n+Specifying `--no-symlinks` instructs 'git difftool' to create copies\n+instead. `--no-symlinks` is the default on Windows.\n \n -x ::\n --extcmd=::\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211251","messageId":"796eafb6816b302c87873c8f4a1bd2225ce40c55.1363206651.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363206651.git.john@keeping.me.uk","subject":"[PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-13T20:33:09Z","receivedAt":"2013-03-13T20:33:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Some users like to edit files in their diff tool when using \"git\ndifftool --dir-diff --symlink\" to compare against the working tree but\ndifftool currently only created symlinks when a file contains unstaged\nchanges.\n\nChange this behaviour so that symlinks are created whenever the\nright-hand side of the comparison has the same SHA1 as the file in the\nworking tree.\n\nNote that textconv filters are handled in the same way as by git-diff\nand if a clean filter is not the inverse of its smudge filter we already\nget a null SHA1 from \"diff --raw\" and will symlink the file without\ngoing through the new hash-object based check.\n\nReported-by: Matt McClure \nSigned-off-by: John Keeping \n---\n Documentation/git-difftool.txt | 4 +++-\n git-difftool.perl | 21 ++++++++++++++++++---\n t/t7800-difftool.sh | 14 ++++++++++++++\n 3 files changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex e575fea..8361e6e 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.\n --symlinks::\n --no-symlinks::\n \t'git difftool''s default behavior is create symlinks to the\n-\tworking tree when run in `--dir-diff` mode.\n+\tworking tree when run in `--dir-diff` mode and the right-hand\n+\tside of the comparison yields the same content as the file in\n+\tthe working tree.\n +\n Specifying `--no-symlinks` instructs 'git difftool' to create copies\n instead. `--no-symlinks` is the default on Windows.\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 0a90de4..5f093ae 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -83,6 +83,21 @@ sub exit_cleanup\n \texit($status | ($status >> 8));\n }\n \n+sub use_wt_file\n+{\n+\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy $null_sha1 = '0' x 40;\n+\n+\tif ($sha1 eq $null_sha1) {\n+\t\treturn 1;\n+\t} elsif (not $symlinks) {\n+\t\treturn 0;\n+\t}\n+\n+\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n+\treturn $sha1 eq $wt_sha1;\n+}\n+\n sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n@@ -159,10 +174,10 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\tif ($rsha1 ne $null_sha1) {\n-\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n-\t\t\t} else {\n+\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n \t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\t} else {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex eb1d3f8..8102ce1 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '\n \techo \"$diff\" | stdin_contains file\n '\n \n+write_script .git/CHECK_SYMLINKS <<\\EOF &&\n+#!/bin/sh\n+test -L \"$2/file\" &&\n+test -L \"$2/file2\" &&\n+test -L \"$2/sub/sub\"\n+echo $?\n+EOF\n+\n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n+\tresult=$(git difftool --dir-diff --symlink \\\n+\t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD) &&\n+\ttest \"$result\" = 0\n+'\n+\n test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n \tdiff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&\n \techo \"$diff\" | stdin_contains sub &&\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211270","messageId":"CAJDDKr5M4pqmd9HSVT8hDJB9AxgV2RexN6B6v6ccTS6raWY_Qg@mail.gmail.com","threadId":"32203","inReplyTo":"796eafb6816b302c87873c8f4a1bd2225ce40c55.1363206651.git.john@keeping.me.uk","subject":"Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-14T03:41:29Z","receivedAt":"2013-03-14T03:41:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 13, 2013 at 1:33 PM, John Keeping wrote:\n> Some users like to edit files in their diff tool when using \"git\n> difftool --dir-diff --symlink\" to compare against the working tree but\n> difftool currently only created symlinks when a file contains unstaged\n> changes.\n>\n> Change this behaviour so that symlinks are created whenever the\n> right-hand side of the comparison has the same SHA1 as the file in the\n> working tree.\n>\n> Note that textconv filters are handled in the same way as by git-diff\n> and if a clean filter is not the inverse of its smudge filter we already\n> get a null SHA1 from \"diff --raw\" and will symlink the file without\n> going through the new hash-object based check.\n>\n> Reported-by: Matt McClure \n> Signed-off-by: John Keeping \n> ---\n> Documentation/git-difftool.txt | 4 +++-\n> git-difftool.perl | 21 ++++++++++++++++++---\n> t/t7800-difftool.sh | 14 ++++++++++++++\n> 3 files changed, 35 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\n> index e575fea..8361e6e 100644\n> --- a/Documentation/git-difftool.txt\n> +++ b/Documentation/git-difftool.txt\n> @@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.\n> --symlinks::\n> --no-symlinks::\n> 'git difftool''s default behavior is create symlinks to the\n> - working tree when run in `--dir-diff` mode.\n> + working tree when run in `--dir-diff` mode and the right-hand\n> + side of the comparison yields the same content as the file in\n> + the working tree.\n> +\n> Specifying `--no-symlinks` instructs 'git difftool' to create copies\n> instead. `--no-symlinks` is the default on Windows.\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 0a90de4..5f093ae 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -83,6 +83,21 @@ sub exit_cleanup\n> exit($status | ($status >> 8));\n> }\n>\n> +sub use_wt_file\n> +{\n> + my ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> + my $null_sha1 = '0' x 40;\n> +\n> + if ($sha1 eq $null_sha1) {\n> + return 1;\n> + } elsif (not $symlinks) {\n> + return 0;\n> + }\n> +\n> + my $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> + return $sha1 eq $wt_sha1;\n> +}\n> +\n> sub setup_dir_diff\n> {\n> my ($repo, $workdir, $symlinks) = @_;\n> @@ -159,10 +174,10 @@ EOF\n> }\n>\n> if ($rmode ne $null_mode) {\n> - if ($rsha1 ne $null_sha1) {\n> - $rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> - } else {\n> + if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> push(@working_tree, $dst_path);\n> + } else {\n> + $rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> }\n> }\n> }\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index eb1d3f8..8102ce1 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '\n> echo \"$diff\" | stdin_contains file\n> '\n>\n> +write_script .git/CHECK_SYMLINKS <<\\EOF &&\n\nTiny nit. Is there any downside to leaving this file\nat the root instead of inside the .git dir?\n\n> +#!/bin/sh\n> +test -L \"$2/file\" &&\n> +test -L \"$2/file2\" &&\n> +test -L \"$2/sub/sub\"\n> +echo $?\n> +EOF\n> +\n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n> + result=$(git difftool --dir-diff --symlink \\\n> + --extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD) &&\n> + test \"$result\" = 0\n> +'\n> +\n\nHow about something like this?\n\n+ echo 0 >expect &&\n+ git difftool --dir-diff --symlink \\\n+ --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&\n+ test_cmp expect actual\n\n(sans gmail whitespace damage) so that we can keep it chained with &&.\nAh.. it seems your branch is based on master, perhaps?\n\nThere's stuff cooking in next for difftool's tests.\nI'm not sure if this patch is based on top of them.\nCan you rebase the tests so that the chaining is done like it is in 'next'?\n-- \nDavid\n"},{"id":"211282","messageId":"20130314093617.GM2317@serenity.lan","threadId":"32203","inReplyTo":"CAJDDKr5M4pqmd9HSVT8hDJB9AxgV2RexN6B6v6ccTS6raWY_Qg@mail.gmail.com","subject":"Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T09:36:17Z","receivedAt":"2013-03-14T09:36:17Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 13, 2013 at 08:41:29PM -0700, David Aguilar wrote:\n> On Wed, Mar 13, 2013 at 1:33 PM, John Keeping wrote:\n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index eb1d3f8..8102ce1 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '\n> > echo \"$diff\" | stdin_contains file\n> > '\n> >\n> > +write_script .git/CHECK_SYMLINKS <<\\EOF &&\n> \n> Tiny nit. Is there any downside to leaving this file\n> at the root instead of inside the .git dir?\n\nI followed what some of the other uses of write_script (in other tests)\ndid. I think putting it under .git is slightly better because it won't\nshow up as untracked in the repository but that shouldn't matter here,\nso I'm happy to change it in a re-roll.\n\n> > +#!/bin/sh\n> > +test -L \"$2/file\" &&\n> > +test -L \"$2/file2\" &&\n> > +test -L \"$2/sub/sub\"\n> > +echo $?\n> > +EOF\n> > +\n> > +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n> > + result=$(git difftool --dir-diff --symlink \\\n> > + --extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD) &&\n> > + test \"$result\" = 0\n> > +'\n> > +\n> \n> How about something like this?\n> \n> + echo 0 >expect &&\n> + git difftool --dir-diff --symlink \\\n> + --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&\n> + test_cmp expect actual\n> \n> (sans gmail whitespace damage) so that we can keep it chained with &&.\n\nI hadn't considered using test_cmp, if we go that way I wonder if we can\ndo slightly better for future debugging. Something like this perhaps?\n\n+write_script .git/CHECK_SYMLINKS <<\\EOF &&\n+for f in file file2 sub/sub\n+do\n+\techo \"$f\"\n+\treadlink \"$2/$f\"\n+done >actual\n+EOF\n+\n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n+\tcat <expect &&\n+file\n+$(pwd)/file\n+file2\n+$(pwd)/file2\n+sub/sub\n+$(pwd)/sub/sub\n+EOF\n+ git difftool --dir-diff --symlink \\\n+ --extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n+\ttest_cmp actual expect\n+'\n\n> Ah.. it seems your branch is based on master, perhaps?\n>\n> There's stuff cooking in next for difftool's tests.\n> I'm not sure if this patch is based on top of them.\n> Can you rebase the tests so that the chaining is done like it is in 'next'?\n\nYes it is based on master. The cleanup on next looks good, I'll base\nthe re-roll on that.\n\n\nJohn\n"},{"id":"211283","messageId":"20130314094300.GN2317@serenity.lan","threadId":"32203","inReplyTo":"7v1ubj45ac.fsf@alter.siamese.dyndns.org","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T09:43:00Z","receivedAt":"2013-03-14T09:43:00Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:\n> Does the temporary checkout correctly apply the smudge filter and\n> crlf conversion, by the way? If not, regardless of the topic in\n> this thread, that may want to be fixed as well. I didn't check.\n\nI've had a look at this and I think it will be much quicker for someone\nmore familiar with git-checkout-index to answer.\n\nWhat git-difftool does is to create a temporary index containing only\nthe files that have changed (using git-update-index --index-info) and\nthen check this out with \"git checkout-index --prefix=...\". So I think\nthis question boils down to: does git-checkout-index still read\n.gitattributes from the working tree if given --prefix?\n\n\nJohn\n"},{"id":"211302","messageId":"7vboamypqh.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"796eafb6816b302c87873c8f4a1bd2225ce40c55.1363206651.git.john@keeping.me.uk","subject":"Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T15:18:14Z","receivedAt":"2013-03-14T15:18:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> +write_script .git/CHECK_SYMLINKS <<\\EOF &&\n> +#!/bin/sh\n> +test -L \"$2/file\" &&\n> +test -L \"$2/file2\" &&\n> +test -L \"$2/sub/sub\"\n> +echo $?\n> +EOF\n\nPlease drop \"#!/bin/sh\" from the above; it is misleading and\npointless.\n\nAfter all, you are using \"write_script\" to avoid having to know\nwhere the user's shell is.\n"},{"id":"211312","messageId":"20130314172515.GB4256@serenity.lan","threadId":"32203","inReplyTo":"20130314094300.GN2317@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T17:25:15Z","receivedAt":"2013-03-14T17:25:15Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Mar 14, 2013 at 09:43:00AM +0000, John Keeping wrote:\n> On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:\n> > Does the temporary checkout correctly apply the smudge filter and\n> > crlf conversion, by the way? If not, regardless of the topic in\n> > this thread, that may want to be fixed as well. I didn't check.\n> \n> What git-difftool does is to create a temporary index containing only\n> the files that have changed (using git-update-index --index-info) and\n> then check this out with \"git checkout-index --prefix=...\". So I think\n> this question boils down to: does git-checkout-index still read\n> .gitattributes from the working tree if given --prefix?\n\nHaving looked at this a bit more, I think it does mostly do the right\nthing, but there is bug in write_entry() that means it won't handle\n.gitattributes correctly when using a streaming filter.\n\nThe path passed to get_stream_filter is only used to decide what filters\napply to the file, so shouldn't it be using \"ce->name\" and not \"path\"\nfor the same reason that the call to convert_to_working_tree() further\ndown the same function does?\n\n-- >8 --\ndiff --git a/entry.c b/entry.c\nindex 17a6bcc..63c52ed 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \tstruct stat st;\n \n \tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\tstruct stream_filter *filter = get_stream_filter(path, ce->sha1);\n+\t\tstruct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);\n \t\tif (filter &&\n \t\t !streaming_write_entry(ce, path, filter,\n \t\t\t\t\t state, to_tempfile,\n"},{"id":"211315","messageId":"7vehfhyjgv.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130314172515.GB4256@serenity.lan","subject":"Re: difftool -d symlinks, under what conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T17:33:36Z","receivedAt":"2013-03-14T17:33:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> The path passed to get_stream_filter is only used to decide what filters\n> apply to the file, so shouldn't it be using \"ce->name\" and not \"path\"\n> for the same reason that the call to convert_to_working_tree() further\n> down the same function does?\n\nCorrect and well spotted.\n\n>\n> -- >8 --\n> diff --git a/entry.c b/entry.c\n> index 17a6bcc..63c52ed 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n> \tstruct stat st;\n> \n> \tif (ce_mode_s_ifmt == S_IFREG) {\n> -\t\tstruct stream_filter *filter = get_stream_filter(path, ce->sha1);\n> +\t\tstruct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);\n> \t\tif (filter &&\n> \t\t !streaming_write_entry(ce, path, filter,\n> \t\t\t\t\t state, to_tempfile,\n"},{"id":"211318","messageId":"cover.1363291173.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"7vehfhyjgv.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] checkout-index: fix .gitattributes handling with --prefix","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:00:49Z","receivedAt":"2013-03-14T20:00:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is from the recent \"difftool --dir-diff\" discussion. With these\npatches applied I think \"difftool --dir-diff\" should correctly apply\nfilters to the files that it checks out with no changes to the\ngit-difftool code.\n\nJohn Keeping (2):\n t2003: modernize style\n entry: fix streaming filter path\n\n entry.c | 2 +-\n t/t2003-checkout-cache-mkdir.sh | 169 +++++++++++++++++++++++-----------------\n 2 files changed, 97 insertions(+), 74 deletions(-)\n\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211319","messageId":"faa93685db024ab3ab196093bc72100cd488ad5f.1363291173.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363291173.git.john@keeping.me.uk","subject":"[PATCH 1/2] t2003: modernize style","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:00:50Z","receivedAt":"2013-03-14T20:00:50Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"- Description goes on the test_expect_* line\n- Open SQ of test goes on the test_expect_* line\n- Closing SQ of test goes on its own line\n- Use TAB for indent\n\nAlso remove three comments that appear to relate to the development of\nthe patch before it was committed.\n\nSigned-off-by: John Keeping \n---\n t/t2003-checkout-cache-mkdir.sh | 143 ++++++++++++++++++++--------------------\n 1 file changed, 70 insertions(+), 73 deletions(-)\n\ndiff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh\nindex 02a4fc5..63fd0a8 100755\n--- a/t/t2003-checkout-cache-mkdir.sh\n+++ b/t/t2003-checkout-cache-mkdir.sh\n@@ -12,85 +12,82 @@ the GIT controlled paths.\n \n . ./test-lib.sh\n \n-test_expect_success \\\n- 'setup' \\\n- 'mkdir path1 &&\n- echo frotz >path0 &&\n- echo rezrov >path1/file1 &&\n- git update-index --add path0 path1/file1'\n+test_expect_success 'setup' '\n+\tmkdir path1 &&\n+\techo frotz >path0 &&\n+\techo rezrov >path1/file1 &&\n+\tgit update-index --add path0 path1/file1\n+'\n \n-test_expect_success SYMLINKS \\\n- 'have symlink in place where dir is expected.' \\\n- 'rm -fr path0 path1 &&\n- mkdir path2 &&\n- ln -s path2 path1 &&\n- git checkout-index -f -a &&\n- test ! -h path1 && test -d path1 &&\n- test -f path1/file1 && test ! -f path2/file1'\n+test_expect_success SYMLINKS 'have symlink in place where dir is expected.' '\n+\trm -fr path0 path1 &&\n+\tmkdir path2 &&\n+\tln -s path2 path1 &&\n+\tgit checkout-index -f -a &&\n+\ttest ! -h path1 && test -d path1 &&\n+\ttest -f path1/file1 && test ! -f path2/file1\n+'\n \n-test_expect_success \\\n- 'use --prefix=path2/' \\\n- 'rm -fr path0 path1 path2 &&\n- mkdir path2 &&\n- git checkout-index --prefix=path2/ -f -a &&\n- test -f path2/path0 &&\n- test -f path2/path1/file1 &&\n- test ! -f path0 &&\n- test ! -f path1/file1'\n+test_expect_success 'use --prefix=path2/' '\n+\trm -fr path0 path1 path2 &&\n+\tmkdir path2 &&\n+\tgit checkout-index --prefix=path2/ -f -a &&\n+\ttest -f path2/path0 &&\n+\ttest -f path2/path1/file1 &&\n+\ttest ! -f path0 &&\n+\ttest ! -f path1/file1\n+'\n \n-test_expect_success \\\n- 'use --prefix=tmp-' \\\n- 'rm -fr path0 path1 path2 tmp* &&\n- git checkout-index --prefix=tmp- -f -a &&\n- test -f tmp-path0 &&\n- test -f tmp-path1/file1 &&\n- test ! -f path0 &&\n- test ! -f path1/file1'\n+test_expect_success 'use --prefix=tmp-' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tgit checkout-index --prefix=tmp- -f -a &&\n+\ttest -f tmp-path0 &&\n+\ttest -f tmp-path1/file1 &&\n+\ttest ! -f path0 &&\n+\ttest ! -f path1/file1\n+'\n \n-test_expect_success \\\n- 'use --prefix=tmp- but with a conflicting file and dir' \\\n- 'rm -fr path0 path1 path2 tmp* &&\n- echo nitfol >tmp-path1 &&\n- mkdir tmp-path0 &&\n- git checkout-index --prefix=tmp- -f -a &&\n- test -f tmp-path0 &&\n- test -f tmp-path1/file1 &&\n- test ! -f path0 &&\n- test ! -f path1/file1'\n+test_expect_success 'use --prefix=tmp- but with a conflicting file and dir' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\techo nitfol >tmp-path1 &&\n+\tmkdir tmp-path0 &&\n+\tgit checkout-index --prefix=tmp- -f -a &&\n+\ttest -f tmp-path0 &&\n+\ttest -f tmp-path1/file1 &&\n+\ttest ! -f path0 &&\n+\ttest ! -f path1/file1\n+'\n \n-# Linus fix #1\n-test_expect_success SYMLINKS \\\n- 'use --prefix=tmp/orary/ where tmp is a symlink' \\\n- 'rm -fr path0 path1 path2 tmp* &&\n- mkdir tmp1 tmp1/orary &&\n- ln -s tmp1 tmp &&\n- git checkout-index --prefix=tmp/orary/ -f -a &&\n- test -d tmp1/orary &&\n- test -f tmp1/orary/path0 &&\n- test -f tmp1/orary/path1/file1 &&\n- test -h tmp'\n+test_expect_success SYMLINKS 'use --prefix=tmp/orary/ where tmp is a symlink' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tmkdir tmp1 tmp1/orary &&\n+\tln -s tmp1 tmp &&\n+\tgit checkout-index --prefix=tmp/orary/ -f -a &&\n+\ttest -d tmp1/orary &&\n+\ttest -f tmp1/orary/path0 &&\n+\ttest -f tmp1/orary/path1/file1 &&\n+\ttest -h tmp\n+'\n \n-# Linus fix #2\n-test_expect_success SYMLINKS \\\n- 'use --prefix=tmp/orary- where tmp is a symlink' \\\n- 'rm -fr path0 path1 path2 tmp* &&\n- mkdir tmp1 &&\n- ln -s tmp1 tmp &&\n- git checkout-index --prefix=tmp/orary- -f -a &&\n- test -f tmp1/orary-path0 &&\n- test -f tmp1/orary-path1/file1 &&\n- test -h tmp'\n+test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tmkdir tmp1 &&\n+\tln -s tmp1 tmp &&\n+\tgit checkout-index --prefix=tmp/orary- -f -a &&\n+\ttest -f tmp1/orary-path0 &&\n+\ttest -f tmp1/orary-path1/file1 &&\n+\ttest -h tmp\n+'\n \n-# Linus fix #3\n-test_expect_success SYMLINKS \\\n- 'use --prefix=tmp- where tmp-path1 is a symlink' \\\n- 'rm -fr path0 path1 path2 tmp* &&\n- mkdir tmp1 &&\n- ln -s tmp1 tmp-path1 &&\n- git checkout-index --prefix=tmp- -f -a &&\n- test -f tmp-path0 &&\n- test ! -h tmp-path1 &&\n- test -d tmp-path1 &&\n- test -f tmp-path1/file1'\n+test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tmkdir tmp1 &&\n+\tln -s tmp1 tmp-path1 &&\n+\tgit checkout-index --prefix=tmp- -f -a &&\n+\ttest -f tmp-path0 &&\n+\ttest ! -h tmp-path1 &&\n+\ttest -d tmp-path1 &&\n+\ttest -f tmp-path1/file1\n+'\n \n test_done\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211320","messageId":"bede6d48dd44f7ed4a11da5821bb112b700475d5.1363291173.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363291173.git.john@keeping.me.uk","subject":"[PATCH 2/2] entry: fix filter lookup","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:00:51Z","receivedAt":"2013-03-14T20:00:51Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When looking up the stream filter, write_entry() should be passing the\npath of the file in the repository, not the path to which the content is\ngoing to be written. This allows the file to be correctly looked up\nagainst the .gitattributes files in the working tree.\n\nThis change makes the streaming case match the non-streaming case which\npasses ce->name to convert_to_working_tree later in the same function.\n\nThe two tests added here test the different paths through write_entry\nsince the CRLF filter is a streaming filter but the user-defined smudge\nfilter is not streamed.\n\nSigned-off-by: John Keeping \n---\n entry.c | 2 +-\n t/t2003-checkout-cache-mkdir.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 17a6bcc..63c52ed 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \tstruct stat st;\n \n \tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\tstruct stream_filter *filter = get_stream_filter(path, ce->sha1);\n+\t\tstruct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);\n \t\tif (filter &&\n \t\t !streaming_write_entry(ce, path, filter,\n \t\t\t\t\t state, to_tempfile,\ndiff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh\nindex 63fd0a8..4c97468 100755\n--- a/t/t2003-checkout-cache-mkdir.sh\n+++ b/t/t2003-checkout-cache-mkdir.sh\n@@ -90,4 +90,30 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '\n \ttest -f tmp-path1/file1\n '\n \n+test_expect_success 'apply filter from working tree .gitattributes with --prefix' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tmkdir path1 &&\n+\tmkdir tmp &&\n+\tgit config filter.replace-all.smudge \"sed -e s/./=/g\" &&\n+\tgit config filter.replace-all.clean cat &&\n+\tgit config filter.replace-all.required true &&\n+\techo \"file1 filter=replace-all\" >path1/.gitattributes &&\n+\tgit checkout-index --prefix=tmp/ -f -a &&\n+\techo frotz >expected &&\n+\ttest_cmp expected tmp/path0 &&\n+\techo ====== >expected &&\n+\ttest_cmp expected tmp/path1/file1\n+'\n+\n+test_expect_success 'apply CRLF filter from working tree .gitattributes with --prefix' '\n+\trm -fr path0 path1 path2 tmp* &&\n+\tmkdir path1 &&\n+\tmkdir tmp &&\n+\techo \"file1 eol=crlf\" >path1/.gitattributes &&\n+\tgit checkout-index --prefix=tmp/ -f -a &&\n+\techo rezrovQ >expected &&\n+\ttr \\\\015 Q actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.2.rc2.4.g7799588\n"},{"id":"211321","messageId":"cover.1363291949.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363206651.git.john@keeping.me.uk","subject":"[PATCH v2 0/3] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:19:38Z","receivedAt":"2013-03-14T20:19:38Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Changes since v1:\n- A new second patch to make it safer to compare symlink targets in\n tests\n- Test in patch 3 (formerly patch 2) re-written thanks to feedback from\n David Aguilar\n\nThis series is based on next, although I think Git's clever enough to\nignore the changes in the context of the t7800 hunk so it should apply\nto master as well.\n\nJohn Keeping (3):\n git-difftool(1): fix formatting of --symlink description\n difftool: avoid double slashes in symlink targets\n difftool --dir-diff: symlink all files matching the working tree\n\n Documentation/git-difftool.txt | 8 +++++---\n git-difftool.perl | 25 +++++++++++++++++++++----\n t/t7800-difftool.sh | 22 ++++++++++++++++++++++\n 3 files changed, 48 insertions(+), 7 deletions(-)\n\n-- \n1.8.2.396.g36b63d6\n"},{"id":"211322","messageId":"3a64f7557df368e986c2a151f04010c76532d4f9.1363291949.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363291949.git.john@keeping.me.uk","subject":"[PATCH v2 1/3] git-difftool(1): fix formatting of --symlink description","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:19:39Z","receivedAt":"2013-03-14T20:19:39Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Signed-off-by: John Keeping \n---\n Documentation/git-difftool.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex e0e12e9..e575fea 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -74,8 +74,8 @@ with custom merge tool commands and has the same value as `$MERGED`.\n \t'git difftool''s default behavior is create symlinks to the\n \tworking tree when run in `--dir-diff` mode.\n +\n-\tSpecifying `--no-symlinks` instructs 'git difftool' to create\n-\tcopies instead. `--no-symlinks` is the default on Windows.\n+Specifying `--no-symlinks` instructs 'git difftool' to create copies\n+instead. `--no-symlinks` is the default on Windows.\n \n -x ::\n --extcmd=::\n-- \n1.8.2.396.g36b63d6\n"},{"id":"211323","messageId":"b10c6d19bd4004887868dd7626320bd676fee540.1363291949.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363291949.git.john@keeping.me.uk","subject":"[PATCH v2 2/3] difftool: avoid double slashes in symlink targets","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:19:40Z","receivedAt":"2013-03-14T20:19:40Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When we add tests for symlinks in \"git difftool --dir-diff\" it's easier\nto check the target path if we don't have to worry about double slashes\nseparating directories. Remove the trailing slash (if present) from\n$workdir before creating the symlinks in order to avoid this.\n\nSigned-off-by: John Keeping \n---\n git-difftool.perl | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 12231fb..e594f9c 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -209,7 +209,9 @@ EOF\n \tdelete($ENV{GIT_INDEX_FILE});\n \n \t# Changes in the working tree need special treatment since they are\n-\t# not part of the index\n+\t# not part of the index. Remove any trailing slash from $workdir\n+\t# before starting to avoid double slashes in symlink targets.\n+\t$workdir =~ s|/$||;\n \tfor my $file (@working_tree) {\n \t\tmy $dir = dirname($file);\n \t\tunless (-d \"$rdir/$dir\") {\n-- \n1.8.2.396.g36b63d6\n"},{"id":"211324","messageId":"ae17a152cadc650920c6446a4493384cc2e77309.1363291949.git.john@keeping.me.uk","threadId":"32203","inReplyTo":"cover.1363291949.git.john@keeping.me.uk","subject":"[PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T20:19:41Z","receivedAt":"2013-03-14T20:19:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Some users like to edit files in their diff tool when using \"git\ndifftool --dir-diff --symlink\" to compare against the working tree but\ndifftool currently only created symlinks when a file contains unstaged\nchanges.\n\nChange this behaviour so that symlinks are created whenever the\nright-hand side of the comparison has the same SHA1 as the file in the\nworking tree.\n\nNote that textconv filters are handled in the same way as by git-diff\nand if a clean filter is not the inverse of its smudge filter we already\nget a null SHA1 from \"diff --raw\" and will symlink the file without\ngoing through the new hash-object based check.\n\nSigned-off-by: John Keeping \n\n---\n Documentation/git-difftool.txt | 4 +++-\n git-difftool.perl | 21 ++++++++++++++++++---\n t/t7800-difftool.sh | 22 ++++++++++++++++++++++\n 3 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex e575fea..8361e6e 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.\n --symlinks::\n --no-symlinks::\n \t'git difftool''s default behavior is create symlinks to the\n-\tworking tree when run in `--dir-diff` mode.\n+\tworking tree when run in `--dir-diff` mode and the right-hand\n+\tside of the comparison yields the same content as the file in\n+\tthe working tree.\n +\n Specifying `--no-symlinks` instructs 'git difftool' to create copies\n instead. `--no-symlinks` is the default on Windows.\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e594f9c..663640d 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -83,6 +83,21 @@ sub exit_cleanup\n \texit($status | ($status >> 8));\n }\n \n+sub use_wt_file\n+{\n+\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy $null_sha1 = '0' x 40;\n+\n+\tif ($sha1 eq $null_sha1) {\n+\t\treturn 1;\n+\t} elsif (not $symlinks) {\n+\t\treturn 0;\n+\t}\n+\n+\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n+\treturn $sha1 eq $wt_sha1;\n+}\n+\n sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n@@ -159,10 +174,10 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\tif ($rsha1 ne $null_sha1) {\n-\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n-\t\t\t} else {\n+\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n \t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\t} else {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 3aab6e1..70e09b6 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '\n \tstdin_contains file actual\n+EOF\n+\n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n+\tcat <expect &&\n+file\n+$(pwd)/file\n+file2\n+$(pwd)/file2\n+sub/sub\n+$(pwd)/sub/sub\n+EOF\n+\tgit difftool --dir-diff --symlink \\\n+\t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n \tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n \tstdin_contains sub writes:\n\n> When we add tests for symlinks in \"git difftool --dir-diff\" it's easier\n> to check the target path if we don't have to worry about double slashes\n> separating directories. Remove the trailing slash (if present) from\n> $workdir before creating the symlinks in order to avoid this.\n\nYup, and it is a good basic hygiene even without tests that expect\nthe exact pathnames.\n\nThe code would work even when your $workdir is at the root of the\nfilesystem; the patch looks good.\n\nThanks.\n\n> Signed-off-by: John Keeping \n> ---\n> git-difftool.perl | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 12231fb..e594f9c 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -209,7 +209,9 @@ EOF\n> \tdelete($ENV{GIT_INDEX_FILE});\n> \n> \t# Changes in the working tree need special treatment since they are\n> -\t# not part of the index\n> +\t# not part of the index. Remove any trailing slash from $workdir\n> +\t# before starting to avoid double slashes in symlink targets.\n> +\t$workdir =~ s|/$||;\n> \tfor my $file (@working_tree) {\n> \t\tmy $dir = dirname($file);\n> \t\tunless (-d \"$rdir/$dir\") {\n"},{"id":"211327","messageId":"7v620ty8lc.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"ae17a152cadc650920c6446a4493384cc2e77309.1363291949.git.john@keeping.me.uk","subject":"Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T21:28:31Z","receivedAt":"2013-03-14T21:28:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 3aab6e1..70e09b6 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '\n> \tstdin_contains file '\n> \n> +write_script .git/CHECK_SYMLINKS <<\\EOF\n> +for f in file file2 sub/sub\n> +do\n> +\techo \"$f\"\n> +\treadlink \"$2/$f\"\n> +done >actual\n> +EOF\n\nWhen you later want to enhance the test to check a combination of\ndifftool arguments where some paths are expected to become links and\nothers are expected to become real files, wouldn't this helper\nbecome a bit awkward to use? The element that expects a real file\ncould be an empty line to what corresponds to the output from\nreadlink, but still...\n\nIf t/ directory (or when the test is run with --root=) is\naliased with symlinks in such a way that \"cd && $(pwd)\" does\nnot match , would this check with $(pwd) still work, I have\nto wonder?\n\n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n> +\tcat <expect &&\n> +file\n> +$(pwd)/file\n> +file2\n> +$(pwd)/file2\n> +sub/sub\n> +$(pwd)/sub/sub\n> +EOF\n\nYou can do this to align them nicer (note the \"-\" before EOF):\n\n\tcat >expect <<-EOF &&\n\tfile\n $(pwd)/file\n ...\n EOF\n\n> +\tgit difftool --dir-diff --symlink \\\n> +\t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n> +\ttest_cmp actual expect\n> +'\n> +\n\nThanks.\n"},{"id":"211329","messageId":"7vwqt9wszu.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"bede6d48dd44f7ed4a11da5821bb112b700475d5.1363291173.git.john@keeping.me.uk","subject":"Re: [PATCH 2/2] entry: fix filter lookup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T21:50:45Z","receivedAt":"2013-03-14T21:50:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh\n> index 63fd0a8..4c97468 100755\n> --- a/t/t2003-checkout-cache-mkdir.sh\n> +++ b/t/t2003-checkout-cache-mkdir.sh\n> @@ -90,4 +90,30 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '\n> \ttest -f tmp-path1/file1\n> '\n> \n> +test_expect_success 'apply filter from working tree .gitattributes with --prefix' '\n> +\trm -fr path0 path1 path2 tmp* &&\n> +\tmkdir path1 &&\n> +\tmkdir tmp &&\n> +\tgit config filter.replace-all.smudge \"sed -e s/./=/g\" &&\n> +\tgit config filter.replace-all.clean cat &&\n> +\tgit config filter.replace-all.required true &&\n> +\techo \"file1 filter=replace-all\" >path1/.gitattributes &&\n> +\tgit checkout-index --prefix=tmp/ -f -a &&\n> +\techo frotz >expected &&\n> +\ttest_cmp expected tmp/path0 &&\n> +\techo ====== >expected &&\n> +\ttest_cmp expected tmp/path1/file1\n> +'\n> +\n> +test_expect_success 'apply CRLF filter from working tree .gitattributes with --prefix' '\n> +\trm -fr path0 path1 path2 tmp* &&\n> +\tmkdir path1 &&\n> +\tmkdir tmp &&\n> +\techo \"file1 eol=crlf\" >path1/.gitattributes &&\n> +\tgit checkout-index --prefix=tmp/ -f -a &&\n> +\techo rezrovQ >expected &&\n> +\ttr \\\\015 Q actual &&\n> +\ttest_cmp expected actual\n> +'\n\nNicely done. Thanks.\n"},{"id":"211332","messageId":"20130314222415.GC4256@serenity.lan","threadId":"32203","inReplyTo":"7v620ty8lc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-14T22:24:15Z","receivedAt":"2013-03-14T22:24:15Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Mar 14, 2013 at 02:28:31PM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> \n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index 3aab6e1..70e09b6 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '\n> > \tstdin_contains file > '\n> > \n> > +write_script .git/CHECK_SYMLINKS <<\\EOF\n> > +for f in file file2 sub/sub\n> > +do\n> > +\techo \"$f\"\n> > +\treadlink \"$2/$f\"\n> > +done >actual\n> > +EOF\n> \n> When you later want to enhance the test to check a combination of\n> difftool arguments where some paths are expected to become links and\n> others are expected to become real files, wouldn't this helper\n> become a bit awkward to use? The element that expects a real file\n> could be an empty line to what corresponds to the output from\n> readlink, but still...\n> \n> If t/ directory (or when the test is run with --root=) is\n> aliased with symlinks in such a way that \"cd && $(pwd)\" does\n> not match , would this check with $(pwd) still work, I have\n> to wonder?\n\nIt looks like t3903 uses \"ls -l\" for this sort of test, perhaps\nsomething like this covers these cases better:\n\n write_script .git/CHECK_SYMLINKS <<\\EOF\n for f in file file2 sub/sub\n do\n ls -l \"$2/$f\" >\"$f\".actual\n done\n EOF\n\n ...\n\n workdir=$(git rev-parse --show-toplevel)\n grep \"-> $workdir/file\" file.actual\n grep \"-> $workdir/file2\" file2.actual\n grep \"-> $workdir/sub/sub\" sub/sub.actual\n\nIt looks like we already rely on that output format in t3903 so I think\nthat is safe, but it would be nice to have a better way to say \"does\nthis link point to that file?\". I can't think of a way to do that that\ndoesn't seem far too complicated for what's required here.\n\n> > +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '\n> > +\tcat <expect &&\n> > +file\n> > +$(pwd)/file\n> > +file2\n> > +$(pwd)/file2\n> > +sub/sub\n> > +$(pwd)/sub/sub\n> > +EOF\n> \n> You can do this to align them nicer (note the \"-\" before EOF):\n> \n> \tcat >expect <<-EOF &&\n> \tfile\n> $(pwd)/file\n> ...\n> EOF\n> \n> > +\tgit difftool --dir-diff --symlink \\\n> > +\t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n> > +\ttest_cmp actual expect\n> > +'\n> > +\n>\n> Thanks.\n"},{"id":"211333","messageId":"7vobelwr3a.fsf@alter.siamese.dyndns.org","threadId":"32203","inReplyTo":"20130314222415.GC4256@serenity.lan","subject":"Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T22:31:53Z","receivedAt":"2013-03-14T22:31:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n>> > +for f in file file2 sub/sub\n>> > +do\n>> > +\techo \"$f\"\n>> > +\treadlink \"$2/$f\"\n>> > +done >actual\n>> > +EOF\n>> \n>> When you later want to enhance the test to check a combination of\n>> difftool arguments where some paths are expected to become links and\n>> others are expected to become real files, wouldn't this helper\n>> become a bit awkward to use? The element that expects a real file\n>> could be an empty line to what corresponds to the output from\n>> readlink, but still...\n>> ...\n>\n> It looks like t3903 uses \"ls -l\" for this sort of test, perhaps\n> something like this covers these cases better:\n> ...\n> grep \"-> $workdir/file\" file.actual\n\nWriting it without -e would confuse some implementations of grep\ninto thinking \"-\" introduces an option, realizing it does not\nsupport the \"->\" option, and then barfing ;-)\n\nWhat I had in mind was more along the lines of...\n\n\tfor f\n do\n \techo \"$f\"\n readlink \"$2/$f\" || echo \"# not a link $f\"\n\tdone\n\nso that your \"expect\" list can become\n\n\tfile\n $(pwd)/realdir/file\n modifiedone.txt\n # not a link modifiedone.txt\n\nIn any case, this \"say blank if you expect a non symlink\" is not an\nurgent issue that needs to be fixed or anything like that, so let's\nqueue the v2 for now and see what happens.\n\nThanks.\n"}]}