{"thread":{"id":"29698","subject":"[PATCH v2] contrib: added git-diffall","startedAt":"2012-02-22T22:12:29Z","lastAt":"2012-04-11T08:38:21Z","messageCount":8,"participants":["Tim Henigan","Junio C Hamano","Stefano Lattarini","Matt McClure","David Aguilar"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"185212","messageId":"1329948749-5908-1-git-send-email-tim.henigan@gmail.com","threadId":"29698","inReplyTo":null,"subject":"[PATCH v2] contrib: added git-diffall","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-02-22T22:12:29Z","receivedAt":"2012-02-22T22:12:29Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"This script adds directory diff support to git.  It launches a single\ninstance of the user-configured external diff tool and performs a\ndirectory diff between the specified revisions. The before/after files\nare copied to a tmp directory to do this.  Either 'diff.tool' or\n'merge.tool' must be set before running the script.\n\nThe existing 'git difftool' command already allows the user to view diffs\nusing an external tool.  However if multiple files have changed, a\nseparate instance of the tool is launched for each one.  This can be\ntedious when many files are involved.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis script has been hosted on GitHub [1] since April 2010. Enough people\nhave found it useful that I hope it will be considered for inclusion in\nthe standard git install, either in contrib or as a new core command.\n\nChanges since v1:\n    - Changed to #!/bin/sh\n    - Eliminated use of 'which' statements\n    - Fixed trap function to actually run on abnormal exit\n    - Simplified path concatenation logic ($IFS)\n    - Corrected indentation errors\n    - Improved readability of while loop\n    - Cleaned up quoting of variables\n\nThis matches commit 5d4b90de3 on GitHub [1].\n\n[1]: https://github.com/thenigan/git-diffall\n\n\n contrib/diffall/README      |   18 +++\n contrib/diffall/git-diffall |  255 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 273 insertions(+)\n create mode 100644 contrib/diffall/README\n create mode 100755 contrib/diffall/git-diffall\n\ndiff --git a/contrib/diffall/README b/contrib/diffall/README\nnew file mode 100644\nindex 0000000..12881d2\n--- /dev/null\n+++ b/contrib/diffall/README\n@@ -0,0 +1,18 @@\n+The git-diffall script provides a directory based diff mechanism\n+for git.  The script relies on the diff.tool configuration option\n+to determine what diff viewer is used.\n+\n+This script is compatible with all the forms used to specify a\n+range of revisions to diff:\n+\n+  1. git diffall: shows diff between working tree and staged changes\n+  2. git diffall --cached [<commit>]: shows diff between staged changes and HEAD (or other named commit)\n+  3. git diffall <commit>: shows diff between working tree and named commit\n+  4. git diffall <commit> <commit>: show diff between two named commits\n+  5. git diffall <commit>..<commit>: same as above\n+  6. git diffall <commit>...<commit>: show the changes on the branch containing and up to the second , starting at a common ancestor of both <commit>\n+\n+Note: all forms take an optional path limiter [--] [<path>*]\n+\n+This script is based on an example provided by Thomas Rast on the Git list [1]:\n+[1] http://thread.gmane.org/gmane.comp.version-control.git/124807\ndiff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall\nnew file mode 100755\nindex 0000000..d942612\n--- /dev/null\n+++ b/contrib/diffall/git-diffall\n@@ -0,0 +1,255 @@\n+#!/bin/sh\n+# Copyright 2010 - 2012, Tim Henigan <tim.henigan@gmail.com>\n+#\n+# Perform a directory diff between commits in the repository using\n+# the external diff or merge tool specified in the user's config.\n+\n+USAGE='[--cached] [--copy-back] [-x|--extcmd=<command>] <commit>{0,2} -- <path>*\n+\n+    --cached     Compare to the index rather than the working tree.\n+\n+    --copy-back  Copy files back to the working tree when the diff\n+                 tool exits (in case they were modified by the\n+                 user).  This option is only valid if the diff\n+                 compared with the working tree.\n+\n+    -x=<command>\n+    --extcmd=<command>  Specify a custom command for viewing diffs.\n+                 git-diffall ignores the configured defaults and\n+                 runs $command $LOCAL $REMOTE when this option is\n+                 specified. Additionally, $BASE is set in the\n+                 environment.\n+'\n+\n+SUBDIRECTORY_OK=1\n+. \"$(git --exec-path)/git-sh-setup\"\n+\n+TOOL_MODE=diff\n+. \"$(git --exec-path)/git-mergetool--lib\"\n+\n+merge_tool=\"$(get_merge_tool)\"\n+if test -z \"$merge_tool\"\n+then\n+\techo \"Error: Either the 'diff.tool' or 'merge.tool' option must be set.\"\n+\tusage\n+fi\n+\n+start_dir=$(pwd)\n+\n+# needed to access tar utility\n+cdup=$(git rev-parse --show-cdup) &&\n+cd \"$cdup\" || {\n+\techo >&2 \"Cannot chdir to $cdup, the toplevel of the working tree\"\n+\texit 1\n+}\n+\n+# mktemp is not available on all platforms (missing from msysgit)\n+# Use a hard-coded tmp dir if it is not available\n+tmp=\"$(mktemp -d -t tmp.XXXXXX 2>/dev/null)\" || {\n+\ttmp=/tmp/git-diffall-tmp\n+}\n+\n+trap 'rm -rf \"$tmp\" 2>/dev/null' EXIT\n+mkdir -p \"$tmp\"\n+\n+left=\n+right=\n+paths=\n+path_sep=\n+compare_staged=\n+common_ancestor=\n+left_dir=\n+right_dir=\n+diff_tool=\n+copy_back=\n+\n+while test $# != 0\n+do\n+\tcase \"$1\" in\n+\t-h|--h|--he|--hel|--help)\n+\t\tusage\n+\t\t;;\n+\t--cached)\n+\t\tcompare_staged=1\n+\t\t;;\n+\t--copy-back)\n+\t\tcopy_back=1\n+\t\t;;\n+\t-x|--e|--ex|--ext|--extc|--extcm|--extcmd)\n+\t\tdiff_tool=$2\n+\t\tshift\n+\t\t;;\n+\t--)\n+\t\tpath_sep=1\n+\t\t;;\n+\t-*)\n+\t\techo Invalid option: \"$1\"\n+\t\tusage\n+\t\t;;\n+\t*)\n+\t\t# could be commit, commit range or path limiter\n+\t\tcase \"$1\" in\n+\t\t*...*)\n+\t\t\tleft=${1%...*}\n+\t\t\tright=${1#*...}\n+\t\t\tcommon_ancestor=1\n+\t\t\t;;\n+\t\t*..*)\n+\t\t\tleft=${1%..*}\n+\t\t\tright=${1#*..}\n+\t\t\t;;\n+\t\t*)\n+\t\t\tif test -n \"$path_sep\"\n+\t\t\tthen\n+\t\t\t\tpaths=\"$paths$1 \"\n+\t\t\telif test -z \"$left\"\n+\t\t\tthen\n+\t\t\t\tleft=$1\n+\t\t\telif test -z \"$right\"\n+\t\t\tthen\n+\t\t\t\tright=$1\n+\t\t\telse\n+\t\t\t\tpaths=\"$paths$1 \"\n+\t\t\tfi\n+\t\t\t;;\n+\t\tesac\n+\t\t;;\n+\tesac\n+\tshift\n+done\n+\n+# Determine the set of files which changed\n+if test -n \"$left\" && test -n \"$right\"\n+then\n+\tleft_dir=\"cmt-$(git rev-parse --short $left)\"\n+\tright_dir=\"cmt-$(git rev-parse --short $right)\"\n+\n+\tif test -n \"$compare_staged\"\n+\tthen\n+\t\tusage\n+\telif test -n \"$common_ancestor\"\n+\tthen\n+\t\tgit diff --name-only \"$left\"...\"$right\" -- $paths > \"$tmp/filelist\"\n+\telse\n+\t\tgit diff --name-only \"$left\" \"$right\" -- $paths > \"$tmp/filelist\"\n+\tfi\n+elif test -n \"$left\"\n+then\n+\tleft_dir=\"cmt-$(git rev-parse --short $left)\"\n+\n+\tif test -n \"$compare_staged\"\n+\tthen\n+\t\tright_dir=\"staged\"\n+\t\tgit diff --name-only --cached \"$left\" -- $paths > \"$tmp/filelist\"\n+\telse\n+\t\tright_dir=\"working_tree\"\n+\t\tgit diff --name-only \"$left\" -- $paths > \"$tmp/filelist\"\n+\tfi\n+else\n+\tleft_dir=\"HEAD\"\n+\n+\tif test -n \"$compare_staged\"\n+\tthen\n+\t\tright_dir=\"staged\"\n+\t\tgit diff --name-only --cached -- $paths > \"$tmp/filelist\"\n+\telse\n+\t\tright_dir=\"working_tree\"\n+\t\tgit diff --name-only -- $paths > \"$tmp/filelist\"\n+\tfi\n+fi\n+\n+# Exit immediately if there are no diffs\n+if test ! -s \"$tmp/filelist\"\n+then\n+\texit 0\n+fi\n+\n+if test -n \"$copy_back\" && test \"$right_dir\" != \"working_tree\"\n+then\n+\techo \"--copy-back is only valid when diff includes the working tree.\"\n+\texit 1\n+fi\n+\n+# Create the named tmp directories that will hold the files to be compared\n+mkdir -p \"$tmp/$left_dir\" \"$tmp/$right_dir\"\n+\n+# Populate the tmp/right_dir directory with the files to be compared\n+if test -n \"$right\"\n+then\n+\twhile read name\n+\tdo\n+\t\tls_list=$(git ls-tree $right $name)\n+\t\tif test -n \"$ls_list\"\n+\t\tthen\n+\t\t\tmkdir -p \"$tmp/$right_dir/$(dirname \"$name\")\"\n+\t\t\tgit show \"$right\":\"$name\" > \"$tmp/$right_dir/$name\" || true\n+\t\tfi\n+\tdone < \"$tmp/filelist\"\n+elif test -n \"$compare_staged\"\n+then\n+\twhile read name\n+\tdo\n+\t\tls_list=$(git ls-files -- $name)\n+\t\tif test -n \"$ls_list\"\n+\t\tthen\n+\t\t\tmkdir -p \"$tmp/$right_dir/$(dirname \"$name\")\"\n+\t\t\tgit show :\"$name\" > \"$tmp/$right_dir/$name\"\n+\t\tfi\n+\tdone < \"$tmp/filelist\"\n+else\n+\t# Mac users have gnutar rather than tar\n+\t(tar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && tar -x)) || {\n+\t\tgnutar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && gnutar -x)\n+\t}\n+fi\n+\n+# Populate the tmp/left_dir directory with the files to be compared\n+while read name\n+do\n+\tif test -n \"$left\"\n+\tthen\n+\t\tls_list=$(git ls-tree $left $name)\n+\t\tif test -n \"$ls_list\"\n+\t\tthen\n+\t\t\tmkdir -p \"$tmp/$left_dir/$(dirname \"$name\")\"\n+\t\t\tgit show \"$left\":\"$name\" > \"$tmp/$left_dir/$name\" || true\n+\t\tfi\n+\telse\n+\t\tif test -n \"$compare_staged\"\n+\t\tthen\n+\t\t\tls_list=$(git ls-tree HEAD $name)\n+\t\t\tif test -n \"$ls_list\"\n+\t\t\tthen\n+\t\t\t\tmkdir -p \"$tmp/$left_dir/$(dirname \"$name\")\"\n+\t\t\t\tgit show HEAD:\"$name\" > \"$tmp/$left_dir/$name\"\n+\t\t\tfi\n+\t\telse\n+\t\t\tmkdir -p \"$tmp/$left_dir/$(dirname \"$name\")\"\n+\t\t\tgit show :\"$name\" > \"$tmp/$left_dir/$name\"\n+\t\tfi\n+\tfi\n+done < \"$tmp/filelist\"\n+\n+cd \"$tmp\"\n+LOCAL=\"$left_dir\"\n+REMOTE=\"$right_dir\"\n+\n+if test -n \"$diff_tool\"\n+then\n+\texport BASE\n+\teval $diff_tool '\"$LOCAL\"' '\"$REMOTE\"'\n+else\n+\trun_merge_tool \"$merge_tool\" false\n+fi\n+\n+# Copy files back to the working dir, if requested\n+if test -n \"$copy_back\" && test \"$right_dir\" = \"working_tree\"\n+then\n+\tcd \"$start_dir\"\n+\tgit_top_dir=$(git rev-parse --show-toplevel)\n+\tfind \"$tmp/$right_dir\" -type f |\n+\twhile read file\n+\tdo\n+\t\tcp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n+\tdone\n+fi\n-- \n1.7.9.GIT\n"},{"id":"185225","messageId":"7vipiy8m5q.fsf@alter.siamese.dyndns.org","threadId":"29698","inReplyTo":"1329948749-5908-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T23:48:33Z","receivedAt":"2012-02-22T23:48:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> This script adds directory diff support to git.  It launches a single\n> instance of the user-configured external diff tool and performs a\n> directory diff between the specified revisions. The before/after files\n> are copied to a tmp directory to do this.  Either 'diff.tool' or\n> 'merge.tool' must be set before running the script.\n>\n> The existing 'git difftool' command already allows the user to view diffs\n> using an external tool.  However if multiple files have changed, a\n> separate instance of the tool is launched for each one.  This can be\n> tedious when many files are involved.\n\nWe encourage our log messages to describe the problem first and then\npresent solution to the problem, so I would update the above perhaps like\nthis:\n\n\tThe 'git difftool' command lets the user to use an external tool\n\tto view diffs, but it runs the tool for one file at the time. This\n\tmakes it tedious to review a change that spans multiple files.\n\n        The \"git-diffall\" script instead prepares temporary directories\n        with preimage and postimage files, and launches a single instance\n        of an external diff tool to view the differences in them.\n        diff.tool or merge.tool configuration variable is used to specify\n        what external tool is used.\n\nI am wondering if reusing \"diff.tool\" or \"merge.tool\" is a good idea,\nthough.\n\nI guess that it is OK to assume that any external tool that can compare\ntwo directories MUST be able to compare two individual files, and if that\nis true, it is perfectly fine to reuse the configuration.  But if an\nexternal tool \"frobdiff\" that can compare two directories cannot compare\ntwo individual files, it will make it impossible for the user to run \"git\ndifftool\" if diff.tool is set to \"frobdiff\" to use with \"diffall\".\n\nAnother thing that comes to my mind is if a user has an external tool that\ncan use \"diffall\", is there ever a situation where the user chooses to use\n\"difftool\" instead, to go files one by one.  I cannot offhand imagine any.\n\nPerhaps a longer term integration plan may be to lift the logic from this\nscript and make it part of \"difftool\", and then add a boolean variable\n\"difftool.<tool>.canCompareDirectory\", without adding \"git diffall\" as a\nsubcommand.  The user can still run \"git difftool\", and when the external\ntool can compare two directories, the code to populate temporary directory\n(and to set the trap to clean after itself) taken from this tool will run\ninside \"git difftool\" frontend and then the external tool to compare the\ntwo directories is spawned.\n\nI also think that in a yet longer term, if this mode of \"instantiate two\ndirectories to be compared, and let the external tool do the comparison\"\nproves useful, almost all the \"interesting\" work done in this script\nshould be made unnecessary by adding an updated \"external diff interface\"\non the core side, so that nobody has to hurt his brain to implement an\nerror-prone command line parsing logic.\n\nIn other words, this statement cannot stay true:\n\n> +This script is compatible with all the forms used to specify a\n> +range of revisions to diff:\n\nwithout updating the script every time underlying \"git diff\" gains new way\nof comparing things.  If we move the \"prepare two directories, and point\nan external tool at them\" logic to the core, we do not have to worry about\nit at all.\n\nBesides, I do not think the script covers all the forms; \"git diff -R\"\nsupport is totally missing.\n\nBut that is all two steps in the future.\n\n> +# mktemp is not available on all platforms (missing from msysgit)\n> +# Use a hard-coded tmp dir if it is not available\n> +tmp=\"$(mktemp -d -t tmp.XXXXXX 2>/dev/null)\" || {\n> +\ttmp=/tmp/git-diffall-tmp\n> +}\n\nIt would not withstand malicious attacks, but doing\n\n\ttmp=/tmp/git-diffall-tmp.$$\n\nwould at least protect you from accidental name crashes better in the\nfallback codepath.\n\n> +\n> +trap 'rm -rf \"$tmp\" 2>/dev/null' EXIT\n\nDo you need to suppress errors, especially when you are running \"rm -rf\"\nwith the 'f' option?\n\n> +mkdir -p \"$tmp\"\n> +\n> +left=\n> +right=\n> +paths=\n> +path_sep=\n> +compare_staged=\n> +common_ancestor=\n> +left_dir=\n> +right_dir=\n> +diff_tool=\n> +copy_back=\n\nYou can write multiple assignment on a line to save vertical space if you\nwant to, and the initialization sequence like this is a good place to do\nso.\n\n> +while test $# != 0\n> +do\n> +\tcase \"$1\" in\n> +\t-h|--h|--he|--hel|--help)\n> +\t\tusage\n> +\t\t;;\n> +\t--cached)\n> +\t\tcompare_staged=1\n> +\t\t;;\n> +\t--copy-back)\n> +\t\tcopy_back=1\n> +\t\t;;\n> +\t-x|--e|--ex|--ext|--extc|--extcm|--extcmd)\n> +\t\tdiff_tool=$2\n> +\t\tshift\n> +\t\t;;\n\nWhat if your command line ends with -x without $2?\n\nDon't you want to match \"-t/--tool\" that \"difftool\" already uses?\n\n> +\t--)\n> +\t\tpath_sep=1\n> +\t\t;;\n> +\t-*)\n> +\t\techo Invalid option: \"$1\"\n> +\t\tusage\n> +\t\t;;\n> +\t*)\n> +\t\t# could be commit, commit range or path limiter\n> +\t\tcase \"$1\" in\n> +\t\t*...*)\n> +\t\t\tleft=${1%...*}\n> +\t\t\tright=${1#*...}\n> +\t\t\tcommon_ancestor=1\n> +\t\t\t;;\n\nStrictly speaking, that is not just a common_ancestor but is a merge_base,\nwhich is a common ancestor none of whose children is a common ancestor.\n\n> +\t\t*..*)\n> +\t\t\tleft=${1%..*}\n> +\t\t\tright=${1#*..}\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tif test -n \"$path_sep\"\n> +\t\t\tthen\n> +\t\t\t\tpaths=\"$paths$1 \"\n> +\t\t\telif test -z \"$left\"\n> +\t\t\tthen\n> +\t\t\t\tleft=$1\n> +\t\t\telif test -z \"$right\"\n> +\t\t\tthen\n> +\t\t\t\tright=$1\n> +\t\t\telse\n> +\t\t\t\tpaths=\"$paths$1 \"\n> +\t\t\tfi\n> +\t\t\t;;\n> +\t\tesac\n\nHrm, so \"diffall HEAD~2 Documentation/\" is not the way to compare the\ncontents of the Documentation/ directory between the named commit and\nthe working tree, like \"diff HEAD~2 Documentation/\" does.\n\nThat is not a show-stopper (a double-dash is an easy workaround), but\nit is worth pointing out.\n\n> +\t\t;;\n> +\tesac\n> +\tshift\n> +done\n> +\n> +# Determine the set of files which changed\n> +if test -n \"$left\" && test -n \"$right\"\n> +then\n> +\tleft_dir=\"cmt-$(git rev-parse --short $left)\"\n> +\tright_dir=\"cmt-$(git rev-parse --short $right)\"\n> +\n> +\tif test -n \"$compare_staged\"\n> +\tthen\n> +\t\tusage\n> +\telif test -n \"$common_ancestor\"\n> +\tthen\n> +\t\tgit diff --name-only \"$left\"...\"$right\" -- $paths > \"$tmp/filelist\"\n> +\telse\n> +\t\tgit diff --name-only \"$left\" \"$right\" -- $paths > \"$tmp/filelist\"\n> +\tfi\n\nAnd this will not work with pathspec that have $IFS characters.  If we\nreally wanted to we could do that by properly quoting \"$1\" when you build\n$paths and then use eval when you run \"git diff\" here (look for 'sq' and\n'eval' in existing scripts, e.g. \"git-am.sh\", if you are interested).\n\nAlso you may want to write filelist using -z format to protect yourself\nfrom paths that contain LF, but that would require the loop \"while read\nname\" to be rewritten.\n\n> +# Exit immediately if there are no diffs\n> +if test ! -s \"$tmp/filelist\"\n> +then\n> +\texit 0\n> +fi\n\nOk, you have trap set already so $tmp will disappear with this exit ;-)\n\n> +if test -n \"$copy_back\" && test \"$right_dir\" != \"working_tree\"\n> +then\n> +\techo \"--copy-back is only valid when diff includes the working tree.\"\n> +\texit 1\n> +fi\n\nI actually wondered why $right_dir needs to be populated with a copy in\nthe first place (if you do not copy but give the working tree itself to\nthe external tool, you do not even have to copy back).\n\nI know the answer to the question, namely, \"because the external tool\nthinks files that are not in $left_dir are added files\", but if you can\nfind a way to tell the external tool to ignore new files (similar to how\n\"diff -r\" without -N works), running the tool with temporary left_dir and\nthe true workdir as right_dir would be a lot cleaner solution to the\nproblem.\n\n> +# Create the named tmp directories that will hold the files to be compared\n> +mkdir -p \"$tmp/$left_dir\" \"$tmp/$right_dir\"\n> +\n> +# Populate the tmp/right_dir directory with the files to be compared\n> +if test -n \"$right\"\n> +then\n> +\twhile read name\n> +\tdo\n> +\t\tls_list=$(git ls-tree $right $name)\n> +\t\tif test -n \"$ls_list\"\n> +\t\tthen\n> +\t\t\tmkdir -p \"$tmp/$right_dir/$(dirname \"$name\")\"\n> +\t\t\tgit show \"$right\":\"$name\" > \"$tmp/$right_dir/$name\" || true\n> +\t\tfi\n> +\tdone < \"$tmp/filelist\"\n\n\"while read -r name\" might make this slightly more robust; even though\nthis loses leading and trailing whitespaces in filenames, we probably\ncan get away without worrying about them.\n\n> +else\n> +\t# Mac users have gnutar rather than tar\n> +\t(tar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && tar -x)) || {\n> +\t\tgnutar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && gnutar -x)\n> +\t}\n\nWhat is this \"--ignore-failed-read\" about?  Not reporting unreadable as an\nerror smells really bad.\n\nIf you require GNUism in your tar usage, this should be made configurable\nso that people can use alternative names (some systems come with \"tar\"\nthat is POSIX and \"gtar\" that is GNU).\n\n> +cd \"$tmp\"\n> +LOCAL=\"$left_dir\"\n> +REMOTE=\"$right_dir\"\n> +\n> +if test -n \"$diff_tool\"\n> +then\n> +\texport BASE\n> +\teval $diff_tool '\"$LOCAL\"' '\"$REMOTE\"'\n> +else\n> +\trun_merge_tool \"$merge_tool\" false\n> +fi\n> +\n> +# Copy files back to the working dir, if requested\n> +if test -n \"$copy_back\" && test \"$right_dir\" = \"working_tree\"\n> +then\n> +\tcd \"$start_dir\"\n> +\tgit_top_dir=$(git rev-parse --show-toplevel)\n> +\tfind \"$tmp/$right_dir\" -type f |\n> +\twhile read file\n> +\tdo\n> +\t\tcp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n> +\tdone\n> +fi\n\nThis will copy new files created in $right_dir.  Is that intended?\n"},{"id":"185254","messageId":"4F460D45.7000804@gmail.com","threadId":"29698","inReplyTo":"7vipiy8m5q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-02-23T09:56:21Z","receivedAt":"2012-02-23T09:56:21Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"Hello everybody.  Hope you don't mind 2 cents from an outsider ...\n\nOn 02/23/2012 12:48 AM, Junio C Hamano wrote:\n>\n> Tim Henigan <tim.henigan@gmail.com> writes:\n> \n>> +# mktemp is not available on all platforms (missing from msysgit)\n>> +# Use a hard-coded tmp dir if it is not available\n>> +tmp=\"$(mktemp -d -t tmp.XXXXXX 2>/dev/null)\" || {\n>> +\ttmp=/tmp/git-diffall-tmp\n>> +}\n> \n> It would not withstand malicious attacks, but doing\n> \n> \ttmp=/tmp/git-diffall-tmp.$$\n> \n> would at least protect you from accidental name crashes better in the\n> fallback codepath.\n>\nMaybe this would be enough to withstand malicious attacks (even if not\ndenial-of-service attacks):\n\n  # mktemp is not available on all platforms (missing from msysgit)\n   tmp=$(mktemp -d -t tmp.XXXXXX 2>/dev/null) || {\n       tmp=/tmp/git-diffall-tmp.$$\n       mkdir \"$tmp\" || fatal \"couldn't create temporary directory\"\n  }\n\n> \n>> +mkdir -p \"$tmp\"\n>\nAt which point this should be removed, of course.\n\nRegards,\n  Stefano\n"},{"id":"185283","messageId":"CAFouetiSpsZGtLt2tG4ou-H18zigNx5xWQH4cy8GrL1eDxbjJw@mail.gmail.com","threadId":"29698","inReplyTo":"7vipiy8m5q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-02-23T16:07:17Z","receivedAt":"2012-02-23T16:07:17Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Feb 22, 2012 at 6:48 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n> We encourage our log messages to describe the problem first and then\n> present solution to the problem, so I would update the above perhaps like\n> this:\n>\n>        The 'git difftool' command lets the user to use an external tool\n>        to view diffs, but it runs the tool for one file at the time. This\n>        makes it tedious to review a change that spans multiple files.\n>\n>        The \"git-diffall\" script instead prepares temporary directories\n>        with preimage and postimage files, and launches a single instance\n>        of an external diff tool to view the differences in them.\n>        diff.tool or merge.tool configuration variable is used to specify\n>        what external tool is used.\n\nUnderstood.  I will update in v3.\n\n\n> I am wondering if reusing \"diff.tool\" or \"merge.tool\" is a good idea,\n> though.\n>\n> I guess that it is OK to assume that any external tool that can compare\n> two directories MUST be able to compare two individual files, and if that\n> is true, it is perfectly fine to reuse the configuration.  But if an\n> external tool \"frobdiff\" that can compare two directories cannot compare\n> two individual files, it will make it impossible for the user to run \"git\n> difftool\" if diff.tool is set to \"frobdiff\" to use with \"diffall\".\n>\n> Another thing that comes to my mind is if a user has an external tool that\n> can use \"diffall\", is there ever a situation where the user chooses to use\n> \"difftool\" instead, to go files one by one.  I cannot offhand imagine any.\n\nIt was my assumption that any tool that supports directory diff also\nsupports individual file diff.  It seems like a strict subset of\ndirectory diff case.\n\n\n> Perhaps a longer term integration plan may be to lift the logic from this\n>\n...snip...\n>\n> But that is all two steps in the future.\n\nI hope that this feature finds it way into the existing core commands.\n This script is intended to be a conversation starter that is also\nimmediately useful as a separate command.  Would it be better to begin\nthe long-term discussion now and skip adding this to contrib?\n\n\n>> +# mktemp is not available on all platforms (missing from msysgit)\n>> +# Use a hard-coded tmp dir if it is not available\n>> +tmp=\"$(mktemp -d -t tmp.XXXXXX 2>/dev/null)\" || {\n>> +     tmp=/tmp/git-diffall-tmp\n>> +}\n>\n> It would not withstand malicious attacks, but doing\n>\n>        tmp=/tmp/git-diffall-tmp.$$\n>\n> would at least protect you from accidental name crashes better in the\n> fallback codepath.\n\nThis makes sense.  I will add a unique portion to the name of the tmp dir in v3.\n\n\n>> +trap 'rm -rf \"$tmp\" 2>/dev/null' EXIT\n>\n> Do you need to suppress errors, especially when you are running \"rm -rf\"\n> with the 'f' option?\n\nOn msysgit, I found that \"rm -rf $tmp\" consistently fails due with a\npermission error.  I don't understand why the script that created the\ntmp dir is not allowed to delete it.  I am still looking into it, but\nso far it appears to be an idiosyncrasy of msysgit.\n\n\n>> +left=\n>> +right=\n>> +paths=\n>> +path_sep=\n>> +compare_staged=\n>> +common_ancestor=\n>> +left_dir=\n>> +right_dir=\n>> +diff_tool=\n>> +copy_back=\n>\n> You can write multiple assignment on a line to save vertical space if you\n> want to, and the initialization sequence like this is a good place to do\n> so.\n\nMy personal preference is to keep them on separate lines.  However if\nthe compressed style is preferred, I will change it.\n\n\n>> +     -x|--e|--ex|--ext|--extc|--extcm|--extcmd)\n>> +             diff_tool=$2\n>> +             shift\n>> +             ;;\n>\n> What if your command line ends with -x without $2?\n\nCurrently it results in a shift error with no useful message to the\nuser.  I will add something for this in v3.\n\n\n> Don't you want to match \"-t/--tool\" that \"difftool\" already uses?\n\nAre you suggesting that I a) change \"-x/--extcmd\" to \"-t/--tool\" or\nthat b) I add support for the \"difftool -t/--tool\" option?\n\nIf \"a\", I was reusing the \"difftool --extcmd\" option which has the\nsame behavior.  If \"b\", I will look into it.\n\n\n>> +             # could be commit, commit range or path limiter\n>> +             case \"$1\" in\n>> +             *...*)\n>> +                     left=${1%...*}\n>> +                     right=${1#*...}\n>> +                     common_ancestor=1\n>> +                     ;;\n>\n> Strictly speaking, that is not just a common_ancestor but is a merge_base,\n> which is a common ancestor none of whose children is a common ancestor.\n\nUnderstood.  I will change the name to \"merge_base\" in v3.\n\n\n>> +             *..*)\n>> +                     left=${1%..*}\n>> +                     right=${1#*..}\n>> +                     ;;\n>> +             *)\n>> +                     if test -n \"$path_sep\"\n>> +                     then\n>> +                             paths=\"$paths$1 \"\n>> +                     elif test -z \"$left\"\n>> +                     then\n>> +                             left=$1\n>> +                     elif test -z \"$right\"\n>> +                     then\n>> +                             right=$1\n>> +                     else\n>> +                             paths=\"$paths$1 \"\n>> +                     fi\n>> +                     ;;\n>> +             esac\n>\n> Hrm, so \"diffall HEAD~2 Documentation/\" is not the way to compare the\n> contents of the Documentation/ directory between the named commit and\n> the working tree, like \"diff HEAD~2 Documentation/\" does.\n>\n> That is not a show-stopper (a double-dash is an easy workaround), but\n> it is worth pointing out.\n\nSo I would need something to determine if a string represents a\ncommit/tag/branch or a path?.  I presume it would need to handle the\ncorner case where a branch/tag and path have the same name.  Is there\nanything like this in the mergetool lib scripts today?\n\n\n>> +     then\n>> +             git diff --name-only \"$left\"...\"$right\" -- $paths > \"$tmp/filelist\"\n>> +     else\n>> +             git diff --name-only \"$left\" \"$right\" -- $paths > \"$tmp/filelist\"\n>> +     fi\n>\n> And this will not work with pathspec that have $IFS characters.  If we\n> really wanted to we could do that by properly quoting \"$1\" when you build\n> $paths and then use eval when you run \"git diff\" here (look for 'sq' and\n> 'eval' in existing scripts, e.g. \"git-am.sh\", if you are interested).\n>\n> Also you may want to write filelist using -z format to protect yourself\n> from paths that contain LF, but that would require the loop \"while read\n> name\" to be rewritten.\n\nI just discovered that the script fails to handle files that have\nspaces in their name, so some further work is needed.\n\n\n>> +# Exit immediately if there are no diffs\n>> +if test ! -s \"$tmp/filelist\"\n>> +then\n>> +     exit 0\n>> +fi\n>\n> Ok, you have trap set already so $tmp will disappear with this exit ;-)\n\nI'll try not to be such a slow learner in the future...but no guarantees.\n\n\n>> +if test -n \"$copy_back\" && test \"$right_dir\" != \"working_tree\"\n>> +then\n>> +     echo \"--copy-back is only valid when diff includes the working tree.\"\n>> +     exit 1\n>> +fi\n>\n> I actually wondered why $right_dir needs to be populated with a copy in\n> the first place (if you do not copy but give the working tree itself to\n> the external tool, you do not even have to copy back).\n>\n> I know the answer to the question, namely, \"because the external tool\n> thinks files that are not in $left_dir are added files\", but if you can\n> find a way to tell the external tool to ignore new files (similar to how\n> \"diff -r\" without -N works), running the tool with temporary left_dir and\n> the true workdir as right_dir would be a lot cleaner solution to the\n> problem.\n\nI'll note this as \"for future consideration\".  I spent some time\ntrying this is the original implementation, but could not find a\nworkable solution in the time I had available.\n\n\n>> +     while read name\n>> +     do\n>> +             ls_list=$(git ls-tree $right $name)\n>> +             if test -n \"$ls_list\"\n>> +             then\n>> +                     mkdir -p \"$tmp/$right_dir/$(dirname \"$name\")\"\n>> +                     git show \"$right\":\"$name\" > \"$tmp/$right_dir/$name\" || true\n>> +             fi\n>> +     done < \"$tmp/filelist\"\n>\n> \"while read -r name\" might make this slightly more robust; even though\n> this loses leading and trailing whitespaces in filenames, we probably\n> can get away without worrying about them.\n>\n>> +else\n>> +     # Mac users have gnutar rather than tar\n>> +     (tar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && tar -x)) || {\n>> +             gnutar --ignore-failed-read -c -T \"$tmp/filelist\" | (cd \"$tmp/$right_dir\" && gnutar -x)\n>> +     }\n>\n> What is this \"--ignore-failed-read\" about?  Not reporting unreadable as an\n> error smells really bad.\n\nIf a file was added or deleted between the two commits being compared,\ntar would fail because a file was missing from \"$tmp/filelist\".  The\n\"--ignore-failed-read\" prevents tar from halting the script in this\ncase.\n\n\n> If you require GNUism in your tar usage, this should be made configurable\n> so that people can use alternative names (some systems come with \"tar\"\n> that is POSIX and \"gtar\" that is GNU).\n\nIs there an example showing how this could be configurable?  The\nproblem is that the \"--ignore-failed-read\" was not supported in all\nflavors of tar.\n\n\n>> +# Copy files back to the working dir, if requested\n>> +if test -n \"$copy_back\" && test \"$right_dir\" = \"working_tree\"\n>> +then\n>> +     cd \"$start_dir\"\n>> +     git_top_dir=$(git rev-parse --show-toplevel)\n>> +     find \"$tmp/$right_dir\" -type f |\n>> +     while read file\n>> +     do\n>> +             cp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n>> +     done\n>> +fi\n>\n> This will copy new files created in $right_dir.  Is that intended?\n\nhmmm...that was not intended.  If would be odd for the user to create\nnew files in this tmp directory, but if the diff tool automatically\ngenerates any files then this could result in unwanted files.\n"},{"id":"185285","messageId":"7v4nuh5u3b.fsf@alter.siamese.dyndns.org","threadId":"29698","inReplyTo":"4F460D45.7000804@gmail.com","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T17:37:44Z","receivedAt":"2012-02-23T17:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefano Lattarini <stefano.lattarini@gmail.com> writes:\n\n>>> +# mktemp is not available on all platforms (missing from msysgit)\n>>> +# Use a hard-coded tmp dir if it is not available\n>>> +tmp=\"$(mktemp -d -t tmp.XXXXXX 2>/dev/null)\" || {\n>>> +\ttmp=/tmp/git-diffall-tmp\n>>> +}\n>>  ...\n>   # mktemp is not available on all platforms (missing from msysgit)\n>    tmp=$(mktemp -d -t tmp.XXXXXX 2>/dev/null) || {\n>        tmp=/tmp/git-diffall-tmp.$$\n>        mkdir \"$tmp\" || fatal \"couldn't create temporary directory\"\n>   }\n>\n>>> +mkdir -p \"$tmp\"\n>>\n> At which point this should be removed, of course.\n\nGood eyes; thanks.\n"},{"id":"185287","messageId":"7vpqd54bks.fsf@alter.siamese.dyndns.org","threadId":"29698","inReplyTo":"CAFouetiSpsZGtLt2tG4ou-H18zigNx5xWQH4cy8GrL1eDxbjJw@mail.gmail.com","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T19:02:59Z","receivedAt":"2012-02-23T19:02:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> On Wed, Feb 22, 2012 at 6:48 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Another thing that comes to my mind is if a user has an external tool that\n>> can use \"diffall\", is there ever a situation where the user chooses to use\n>> \"difftool\" instead, to go files one by one. I cannot offhand imagine any.\n>\n> It was my assumption that any tool that supports directory diff also\n> supports individual file diff.  It seems like a strict subset of\n> directory diff case.\n> ...\n>> Perhaps a longer term integration plan may be to lift the logic from this\n>>\n> ...snip...\n>>\n>> But that is all two steps in the future.\n>\n> I hope that this feature finds it way into the existing core commands.\n> This script is intended to be a conversation starter that is also\n> immediately useful as a separate command.  Would it be better to begin\n> the long-term discussion now and skip adding this to contrib?\n\nI would envision we have this in contrib/ first, without even fixing the\nwhitespace-in-pathspec and whitespace-or-lf-in-paths issues I pointed out\nin my review, and let people play with it.\n\nMy crystal ball tells an optimist in me that we will see people (you and\nothers) try to fix issues they hit in their real life use cases, and the\nscript will be improved while it is still in contrib/.  And then somebody\nwho has worked on difftool will step up and roll it into difftool proper,\nalong the lines I hinted in the message you are responding to, at which\npoint the script will be removed from contrib/.  That is the first step in\nthe future.\n\nThe second step in the future may or may not come.  It will involve adding\nan updated external diff interface on the core side and would prepare the\ntwo temporary directories before the core side calls the difftool among\nother things, and when that happens, we can lose most of the code in this\nscript that the first step in the future may have ported into difftool.\n\n>> You can write multiple assignment on a line to save vertical space if you\n>> want to, and the initialization sequence like this is a good place to do\n>> so.\n> My personal preference is to keep them on separate lines.  However if\n> the compressed style is preferred, I will change it.\n\n\"I wouldn't bother\" was what I meant by \"if you want to\".\n\n>>> +     -x|--e|--ex|--ext|--extc|--extcm|--extcmd)\n> ...\n>> Don't you want to match \"-t/--tool\" that \"difftool\" already uses?\n>\n> Are you suggesting that I a) change \"-x/--extcmd\" to \"-t/--tool\" or\n> that b) I add support for the \"difftool -t/--tool\" option?\n\nNo, I just misread the set of options \"difftool\" takes, without realizing\nthat --extcmd and --tool are two different things, and it is correct to\ncall your option \"--extcmd\" if it specifies what corresponds to what\n\"difftool --extcmd\" specifies.  Sorry for the confusion.\n\n>> Hrm, so \"diffall HEAD~2 Documentation/\" is not the way to compare the\n>> contents of the Documentation/ directory between the named commit and\n>> the working tree, like \"diff HEAD~2 Documentation/\" does.\n>>\n>> That is not a show-stopper (a double-dash is an easy workaround), but\n>> it is worth pointing out.\n>\n> So I would need something to determine if a string represents a\n> commit/tag/branch or a path?.\n\n\"It does not have to be fixed in the first version\" (aka \"for future\nconsideration\") was what I meant by \"not a show-stopper\".\n\n>> And this will not work with pathspec that have $IFS characters.  If we\n>> really wanted to we could do that by properly quoting \"$1\" when you build\n>> $paths and then use eval when you run \"git diff\" here (look for 'sq' and\n>> 'eval' in existing scripts, e.g. \"git-am.sh\", if you are interested).\n>>\n>> Also you may want to write filelist using -z format to protect yourself\n>> from paths that contain LF, but that would require the loop \"while read\n>> name\" to be rewritten.\n>\n> I just discovered that the script fails to handle files that have\n> spaces in their name, so some further work is needed.\n\nAgain, \"It does not have to be fixed in the first version\" (aka \"for\nfuture consideration\") was what I meant by \"If we really wanted to\".\n\n>> What is this \"--ignore-failed-read\" about? Not reporting unreadable as an\n>> error smells really bad.\n>\n> If a file was added or deleted between the two commits being compared,\n> tar would fail because a file was missing from \"$tmp/filelist\".  The\n> \"--ignore-failed-read\" prevents tar from halting the script in this\n> case.\n\nBut it will also ignore errors coming from other causes, no?  Wouldn't we\nrather see an error if tar fails to read from a path that *has to* exist\nin the working tree because \"diff\" said it does?\n\nAgain, it is just \"for future consideration\".\n\n>> If you require GNUism in your tar usage, this should be made configurable\n>> so that people can use alternative names (some systems come with \"tar\"\n>> that is POSIX and \"gtar\" that is GNU).\n>\n> Is there an example showing how this could be configurable?  The\n> problem is that the \"--ignore-failed-read\" was not supported in all\n> flavors of tar.\n\nGrep for \"$TAR\" and also @@DIFF@@ in the Makefile, and add substitution\nfor @@TAR@@ in cmd_munge_script, perhaps?\n\nBy the way, I actually have an even more radical suggestion that may let\nyou get rid of most of the lines in your script.\n\nIf you tweak the usage so that \"diffall\" specific options *MUST* come\nfirst, e.g.\n\n\tUSAGE=[--copy-back] [-x <cmd>] <arguments for diff>\n\nthen you can parse your argument partially, i.e.\n\n\tcopy_back= extcmd=\n\twhile case \"$#,$1\" in 0,*) break ;; *,-*) ;; *) break ;; esac\n        do\n\t\tcase \"$1\" in\n                --copy-back)\n                \tcopy_back=true\n\t\t\t;;\n                -x | --extcmd)\n\t\t\ttest $# != 1 || usage\n\t\t\textcmd=$2\n                        shift\n\t\t\t;;\n\t\t*)\n                \tbreak\n                        ;;\n\t\tesac\n                shift\n\tdone\n\nthen feed the remainder all to \"diff\", e.g.\n\n\tdiff --raw --no-abbrev \"$@\" |\n\nAnd then you can prepare two temporary index files and stuff the output in\nthem, by having something like this on the downstream of the pipe:\n\n\twhile read -r lmode rmode lsha1 rsha1 status path\n        do\n        \tif test \"$lmode\" != $null_mode\n\t\tthen\n                \tGIT_INDEX_FILE=$tmp.left_index \\\n                        git update-index --add --cacheinfo $lmode $lsha1 $path\n\t\tfi\n        \tif test \"$rmode\" != $null_mode\n\t\tthen\n                \tGIT_INDEX_FILE=$tmp.right_index \\\n                        git update-index --add --cacheinfo $rmode $rsha1 $path\n\t\tfi\n\tdone\n\nSide Note:\n\tIn the production version, you would probably give the \"-z\" option\n\tto \"diff\", and write this loop in Perl so that you can cope better\n\twith funny characters in the path.  Instead of running two\n\tinstances of \"git update-index\" for every path, the loop would\n\tgroup the entries for left and right side, and drive one instance\n\tof \"git update-index --index-info\" each to populate the two index\n\tfiles.\n\n\tAlso the above needs to be adjusted to deal with the side that\n\trepresents the working tree files; they are reported with $null_sha1\n\tso in such a case instead of putting it in the temporary index,\n        you would copy the working tree file to the temporary location.\n\nAfter you prepare these two temporary index files, you can then use them\nto populate your left_dir and right_dir like this:\n\n\tGIT_DIR=$(git rev-parse --git-dir) \\\n\tGIT_WORK_TREE=$left_dir \\\n        GIT_INDEX_FILE=$tmp.left_index \\\n        git checkout-index -a\n\nWith this, you do not have to worry about anything about the funny\ncombinations of where the two \"directories\" comes from when preparing\nthe temporary directories to be compared.\n"},{"id":"188916","messageId":"loom.20120411T010200-132@post.gmane.org","threadId":"29698","inReplyTo":"CAFouetiSpsZGtLt2tG4ou-H18zigNx5xWQH4cy8GrL1eDxbjJw@mail.gmail.com","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2012-04-10T23:06:43Z","receivedAt":"2012-04-10T23:06:43Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"Tim Henigan <tim.henigan <at> gmail.com> writes:\n\n> >> +     do\n> >> +             cp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n> >> +     done\n> >> +fi\n> >\n> > This will copy new files created in $right_dir.  Is that intended?\n> \n> hmmm...that was not intended.  If would be odd for the user to create\n> new files in this tmp directory, but if the diff tool automatically\n> generates any files then this could result in unwanted files.\n\nI think more generally, I would prefer if either side of the comparison is the\nworking copy that the temp directory on that side be populated with symlinks.\n\nA particularly bad failure mode of the copy-back approach is:\n\ngit diffall --copy-back\n# while my diffall tool is running, I edit the file somewhere else.\n# quit my diffall tool\n# --> my edits in the other tool are overwritten by diffall\n\nEditing the files in place via symlinks would resolve that.\n\nMatt\n"},{"id":"188934","messageId":"CAJDDKr47BZ=QE_nUqAoFJTRTBUxMHD2QwmqpGYrXb3q1hfyAHA@mail.gmail.com","threadId":"29698","inReplyTo":"loom.20120411T010200-132@post.gmane.org","subject":"Re: [PATCH v2] contrib: added git-diffall","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-04-11T08:38:21Z","receivedAt":"2012-04-11T08:38:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Apr 10, 2012 at 4:06 PM, Matt McClure <matthewlmcclure@gmail.com> wrote:\n> Tim Henigan <tim.henigan <at> gmail.com> writes:\n>\n>> >> +     do\n>> >> +             cp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n>> >> +     done\n>> >> +fi\n>> >\n>> > This will copy new files created in $right_dir.  Is that intended?\n>>\n>> hmmm...that was not intended.  If would be odd for the user to create\n>> new files in this tmp directory, but if the diff tool automatically\n>> generates any files then this could result in unwanted files.\n>\n> I think more generally, I would prefer if either side of the comparison is the\n> working copy that the temp directory on that side be populated with symlinks.\n>\n> A particularly bad failure mode of the copy-back approach is:\n>\n> git diffall --copy-back\n> # while my diffall tool is running, I edit the file somewhere else.\n> # quit my diffall tool\n> # --> my edits in the other tool are overwritten by diffall\n>\n> Editing the files in place via symlinks would resolve that.\n\nI had a similar idea but didn't mention it because Windows came to\nmind.  I always want to say, \"darn it, this code would be so much\neasier if we could just ignore Windows\", but that's not very helpful.\n\nI'd be happy with a runtime platform check where the copy back is only\ndone on Windows.  Everyone else can enjoy symlinks.\n\nReading between the lines that could be interpreted as, \"well, that\ncopy back code is no good and *we* don't want to use it, but it's okay\nfor Windows users\", which is slightly dangerous because we'd always be\nrunning the symlink code path and wouldn't hit problems with the other\npath.\n\nSo I'm torn.  I think symlinks are a great idea, but Windows drives us\ntowards the less-than-ideal solution.  I want the best solution\npossible.  Do we just accept that the copy-back code is simply the\ncost of supporting Windows and keep both code paths around?  I would\nnot be opposed to that if the result is a more robust user experience.\n-- \nDavid\n"}]}