{"thread":{"id":"29677","subject":"[PATCH] contrib: added git-diffall","startedAt":"2012-02-21T00:59:29Z","lastAt":"2012-02-22T21:06:14Z","messageCount":8,"participants":["Tim Henigan","Junio C Hamano","David Aguilar","Stefano Lattarini"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185050","messageId":"1329785969-828-1-git-send-email-tim.henigan@gmail.com","threadId":"29677","inReplyTo":null,"subject":"[PATCH] contrib: added git-diffall","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-02-21T00:59:29Z","receivedAt":"2012-02-21T00:59: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\n[1]: https://github.com/thenigan/git-diffall\n\n contrib/diffall/README      |   18 +++\n contrib/diffall/git-diffall |  275 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 293 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..ef01eda\n--- /dev/null\n+++ b/contrib/diffall/git-diffall\n@@ -0,0 +1,275 @@\n+#!/bin/bash -e\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+if test -z $(which mktemp 2>/dev/null)\n+then\n+\ttmp=/tmp/git-diffall-tmp\n+else\n+\ttmp=\"$(mktemp -d -t tmp.XXXXXX)\"\n+fi\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\tif test -z \"$paths\"\n+\t\t\t\tthen\n+\t\t\t\t\tpaths=$1\n+\t\t\t\telse\n+\t\t\t\t\tpaths=\"$paths $1\"\n+\t\t\t\tfi\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\tif test -z \"$paths\"\n+\t\t\t\tthen\n+\t\t\t\t\tpaths=$1\n+\t\t\t\telse\n+\t\t\t\t\tpaths=\"$paths $1\"\n+\t\t\t\tfi\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+\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+\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+\tif test -n \"$(which gnutar 2>/dev/null)\"\n+\tthen\n+\t\tgnutar --ignore-failed-read -c -T \"$tmp\"/filelist | (cd \"$tmp\"/\"$right_dir\" && gnutar -x)\n+\telse\n+\t\ttar --ignore-failed-read -c -T \"$tmp\"/filelist | (cd \"$tmp\"/\"$right_dir\" && tar -x)\n+\tfi\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\t\tfi\n+\t\t\telse\n+\t\t\t\tmkdir -p \"$tmp\"/\"$left_dir\"/\"$(dirname \"$name\")\"\n+\t\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+# This function is called on exit\n+cleanup () {\n+\tcd \"$start_dir\"\n+\n+\t# Copy files back to the working dir, if requested\n+\tif test -n \"$copy_back\" && test \"$right_dir\" = \"working_tree\"\n+\tthen\n+\t\tgit_top_dir=$(git rev-parse --show-toplevel)\n+\t\tfind \"$tmp/$right_dir\" -type f|while read file; do\n+\t\t\tcp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n+\t\tdone\n+\tfi\n+\n+\t# Remove the tmp directory\n+\trm -rf \"$tmp\"\n+}\n+\n+trap cleanup EXIT\n-- \n1.7.9.GIT\n"},{"id":"185106","messageId":"7vd397g8ic.fsf@alter.siamese.dyndns.org","threadId":"29677","inReplyTo":"1329785969-828-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T21:51:39Z","receivedAt":"2012-02-21T21:51:39Z","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> diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall\n> new file mode 100755\n> index 0000000..ef01eda\n> --- /dev/null\n> +++ b/contrib/diffall/git-diffall\n> @@ -0,0 +1,275 @@\n> +#!/bin/bash -e\n\nDoes this have to be bash-only (iow, infested with bash-isms beyond\nrepair), or is you wrote this merely from inertia?\n\nThe following is only after a cursory scanning, so there may be other\nthings that needs fixing, but anyway:\n\n - Don't use \"which\" in scripts.  Its output is not machine parseable, and\n   exit code is not reliable, across platforms.  It is only meant for\n   consumption by human who can read English (or natural language in the\n   current locale).\n\n> +\t\t\t\tif test -z \"$paths\"\n> +\t\t\t\tthen\n> +\t\t\t\t\tpaths=$1\n> +\t\t\t\telse\n> +\t\t\t\t\tpaths=\"$paths $1\"\n> +\t\t\t\tfi\n\nJust a style tip; if you are going to let shell $IFS split this list\nanyway, it is customary to write the above as\n\n\tpaths=\"$paths$1 \"\n\n> +\t\tgit diff --name-only \"$left\"...\"$right\" -- $paths > \"$tmp\"/filelist\n\n\tgit diff ... -- $paths >\"$tmp/filelist\"\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\tmkdir -p \"$tmp/$left_dir\" \"$tmp/$right_dir\"\n\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\t\tfi\n> +\t\t\telse\n> +\t\t\t\tmkdir -p \"$tmp\"/\"$left_dir\"/\"$(dirname \"$name\")\"\n> +\t\t\t\tgit show :\"$name\" > \"$tmp\"/\"$left_dir\"/\"$name\"\n> +\t\tfi\n\nThat's misleadingly indented.  First I thought \"in what case would we want\nto switch the LHS between HEAD:$path and :$path when doing diff --cached?\"\nbut the overindented four lines starting from the funny \"fi\" is about non\ncached case.\n\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> +# This function is called on exit\n> +cleanup () {\n> +\tcd \"$start_dir\"\n> +\n> +\t# Copy files back to the working dir, if requested\n> +\tif test -n \"$copy_back\" && test \"$right_dir\" = \"working_tree\"\n> +\tthen\n> +\t\tgit_top_dir=$(git rev-parse --show-toplevel)\n> +\t\tfind \"$tmp/$right_dir\" -type f|while read file; do\n> +\t\t\tcp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n> +\t\tdone\n\nWhy is this loop written in such a dense way?  Everything else (except for\nthat misindented part) were almost to our CodingStyle and was fairly easy\nto read, though.\n\n> +\tfi\n> +\n> +\t# Remove the tmp directory\n> +\trm -rf \"$tmp\"\n> +}\n> +\n> +trap cleanup EXIT\n\nDoes this even trigger?  This is not Perl that parses and runs set-up code\nbefore executing everything else, so I suspect this last line amounts to\nthe same thing as writing just\n\n\tcleanup\n\nwithout trap nor signal names.\n\nIf you are to set up temporary files or directories that you want to clean\nup, a good discipline is to follow this order:\n\n  - define variable(s) to hold the temporary locations, e.g.\n    tmpdir=$(mktemp ...)\n\n  - set the trap before starting to use these temporary locations, e.g.\n    trap 'rm -rf \"$tmpdir' 0 1 2 3 15\n\n  - and then start populating tmpdir and do whatever you want to do.\n"},{"id":"185137","messageId":"CAFouetiLmK3dXLRkBh+cTNA_OMPS77xo8z95WK5y4tk-o-UUog@mail.gmail.com","threadId":"29677","inReplyTo":"7vd397g8ic.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-02-22T02:02:43Z","receivedAt":"2012-02-22T02:02:43Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Thank you for taking the time to review this patch.  I appreciate it.\n\n\nOn Tue, Feb 21, 2012 at 4:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n\n>> +#!/bin/bash -e\n>\n> Does this have to be bash-only (iow, infested with bash-isms beyond\n> repair), or is you wrote this merely from inertia?\n\nThere is no specific reason it must be bash.  I changed from\n\"#!/bin/sh\" to \"#!/bin/bash -e\" due to a bug report from a user on\nUbuntu [1].  The user reported that:\n\n    \"If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.\n    This causes git_merge_tool_path to fail. The error isn't trapped,\nso it exits\n    without displaying anything and without cleaning up.\"\n\nGiven that all the other scripts distributed with git use /bin/sh, I\nwill change this script to match.\n\n\n> The following is only after a cursory scanning, so there may be other\n> things that needs fixing, but anyway:\n>\n>  - Don't use \"which\" in scripts.  Its output is not machine parseable, and\n>   exit code is not reliable, across platforms.  It is only meant for\n>   consumption by human who can read English (or natural language in the\n>   current locale).\n\nI used \"which\" in two places.  Both were added to support problems\nwith missing standard tools on certain platforms (missing mktemp on\nmsysgit and missing option from tar on Mac [2]).  Is there some other\nstandard way to detect the platform or if certain utils are present?\n\n>> +                             if test -z \"$paths\"\n>> +                             then\n>> +                                     paths=$1\n>> +                             else\n>> +                                     paths=\"$paths $1\"\n>> +                             fi\n>\n> Just a style tip; if you are going to let shell $IFS split this list\n> anyway, it is customary to write the above as\n>\n>        paths=\"$paths$1 \"\n\nNice...I will change to use this format.\n\n\n>> +             git diff --name-only \"$left\"...\"$right\" -- $paths > \"$tmp\"/filelist\n>\n>        git diff ... -- $paths >\"$tmp/filelist\"\n... <snip>\n>        mkdir -p \"$tmp/$left_dir\" \"$tmp/$right_dir\"\n\nWill change all instances.\n\n\n>> +             if test -n \"$compare_staged\"\n>> +             then\n>> +                     ls_list=$(git ls-tree HEAD $name)\n>> +                     if test -n \"$ls_list\"\n>> +                     then\n>> +                             mkdir -p \"$tmp\"/\"$left_dir\"/\"$(dirname \"$name\")\"\n>> +                             git show HEAD:\"$name\" > \"$tmp\"/\"$left_dir\"/\"$name\"\n>> +                             fi\n>> +                     else\n>> +                             mkdir -p \"$tmp\"/\"$left_dir\"/\"$(dirname \"$name\")\"\n>> +                             git show :\"$name\" > \"$tmp\"/\"$left_dir\"/\"$name\"\n>> +             fi\n>\n> That's misleadingly indented.  First I thought \"in what case would we want\n> to switch the LHS between HEAD:$path and :$path when doing diff --cached?\"\n> but the overindented four lines starting from the funny \"fi\" is about non\n> cached case.\n\nThat is embarrassing.  I will fix it.\n\n<snip>\n\n>> +             find \"$tmp/$right_dir\" -type f|while read file; do\n>> +                     cp \"$file\" \"$git_top_dir/${file#$tmp/$right_dir/}\"\n>> +             done\n>\n> Why is this loop written in such a dense way?  Everything else (except for\n> that misindented part) were almost to our CodingStyle and was fairly easy\n> to read, though.\n\nI missed this in my style cleanup.  I will fix it.\n\n\n>> +     fi\n>> +\n>> +     # Remove the tmp directory\n>> +     rm -rf \"$tmp\"\n>> +}\n>> +\n>> +trap cleanup EXIT\n>\n> Does this even trigger?  This is not Perl that parses and runs set-up code\n> before executing everything else, so I suspect this last line amounts to\n> the same thing as writing just\n>\n>        cleanup\n>\n> without trap nor signal names.\n\nThe cleanup triggers on all the platforms I have tested (Ubuntu,\nmsysgit, Mac).  I could change it, but for me it has \"just worked\".\n\n\n> If you are to set up temporary files or directories that you want to clean\n> up, a good discipline is to follow this order:\n>\n>  - define variable(s) to hold the temporary locations, e.g.\n>    tmpdir=$(mktemp ...)\n>\n>  - set the trap before starting to use these temporary locations, e.g.\n>    trap 'rm -rf \"$tmpdir' 0 1 2 3 15\n>\n>  - and then start populating tmpdir and do whatever you want to do.\n\nI will review the changes needed for this before submitting v2 of my patch.\n\n[1]: https://github.com/thenigan/git-diffall/pull/9\n[2]: https://github.com/thenigan/git-diffall/pull/2#issuecomment-498472\n"},{"id":"185143","messageId":"7vbooregj6.fsf@alter.siamese.dyndns.org","threadId":"29677","inReplyTo":"CAFouetiLmK3dXLRkBh+cTNA_OMPS77xo8z95WK5y4tk-o-UUog@mail.gmail.com","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T02:41:17Z","receivedAt":"2012-02-22T02:41:17Z","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> There is no specific reason it must be bash.  I changed from\n> \"#!/bin/sh\" to \"#!/bin/bash -e\" due to a bug report from a user on\n> Ubuntu [1].  The user reported that:\n>\n>     \"If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.\n>     This causes git_merge_tool_path to fail. The error isn't trapped,\n> so it exits\n>     without displaying anything and without cleaning up.\"\n>\n> Given that all the other scripts distributed with git use /bin/sh, I\n> will change this script to match.\n\nYou need to dig back to that bug report deeper and find out what exactly\nis causing the \"failure\", before blindly allowing /bin/dash to be used.\n\nI think the above function name is a typo of get_merge_tool_path that is\nborrowed from git-mergetool--lib.sh, but nothing in the function jumps as\na blatant bash-ism at me from a quick reading.\n\nDavid, any idea on this?\n\n>> The following is only after a cursory scanning, so there may be other\n>> things that needs fixing, but anyway:\n>>\n>>  - Don't use \"which\" in scripts.  Its output is not machine parseable, and\n>>   exit code is not reliable, across platforms.  It is only meant for\n>>   consumption by human who can read English (or natural language in the\n>>   current locale).\n>\n> I used \"which\" in two places.  Both were added to support problems\n> with missing standard tools on certain platforms (missing mktemp on\n> msysgit and missing option from tar on Mac [2]).  Is there some other\n> standard way to detect the platform or if certain utils are present?\n\nThere are examples in the script you are borrowing functions from, even in\nthe function that allegedly fail for the dash user ;-).\n\n> The cleanup triggers on all the platforms I have tested (Ubuntu,\n> msysgit, Mac).  I could change it, but for me it has \"just worked\".\n\nYou set \"trap cleanup\" for exit event, and then the control reaches the\nend of the script, which is an exit event, and the cleanup function is\ncalled.  So it is natural that if you manage to get to that \"trap cleanup\"\nline, of course cleanup will run.  But if you dropped \"trap\" and \"EXIT\"\nfrom that line, it amounts to the same thing.\n\nA more important thing to know is that until the control reaches that\n\"trap cleanup EXIT\" line, you do not have that trap set.  So if you caused\nthe program to exit in an earlier part of the program (say, in the big\n\"while read name\" loop) before the control reaches this line, you won't\nsee any cleanup.\n\nPerhaps that is what you wanted, but then again writing \"trap\" and \"EXIT\"\nthere is pointless.\n\nUsually people set up a clean-up task with trap before going into complex\nstuff, so that no matter where in the complex code the trapped condition\n(typically a signal, but an exit event is also possible) happens, they can\nbe sure the clean-up task is run.  That is the reason behind the following\nordering.\n\n>> If you are to set up temporary files or directories that you want to clean\n>> up, a good discipline is to follow this order:\n>>\n>>  - define variable(s) to hold the temporary locations, e.g.\n>>    tmpdir=$(mktemp ...)\n>>\n>>  - set the trap before starting to use these temporary locations, e.g.\n>>    trap 'rm -rf \"$tmpdir' 0 1 2 3 15\n>>\n>>  - and then start populating tmpdir and do whatever you want to do.\n"},{"id":"185159","messageId":"CAJDDKr72rPqp1z-12Ht3Q2UaeUVFutKwoOgkD1G+SbhsBs+p1A@mail.gmail.com","threadId":"29677","inReplyTo":"7vbooregj6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-02-22T09:15:17Z","receivedAt":"2012-02-22T09:15:17Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n>> There is no specific reason it must be bash.  I changed from\n>> \"#!/bin/sh\" to \"#!/bin/bash -e\" due to a bug report from a user on\n>> Ubuntu [1].  The user reported that:\n>>\n>>     \"If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.\n>>     This causes git_merge_tool_path to fail. The error isn't trapped,\n>> so it exits\n>>     without displaying anything and without cleaning up.\"\n>>\n>> Given that all the other scripts distributed with git use /bin/sh, I\n>> will change this script to match.\n>\n> You need to dig back to that bug report deeper and find out what exactly\n> is causing the \"failure\", before blindly allowing /bin/dash to be used.\n>\n> I think the above function name is a typo of get_merge_tool_path that is\n> borrowed from git-mergetool--lib.sh, but nothing in the function jumps as\n> a blatant bash-ism at me from a quick reading.\n>\n> David, any idea on this?\n\nI don't see any bash-isms there myself, either.  We should keep this\nstuff without bash-isms.\nI haven't had time to read these patches in depth yet but will try to\nread the re-roll.\n\nCan we ask the github user to elaborate on what exactly was erroring out?\nDoes dash not handle || inside $()?  We can only make wild guesses\nwithout their help.\n\nThe only hint from the pull request is \"silent exit with no results\".\nDo we do that?\nThere are a few code paths where we do \"exit 1\" but that's only under\nerror conditions.\n\nWe haven't had any reports about git-mergetool/difftool, which use\nthese functions...\nare we certain the problem was not some other bash-isms in the script?\n-- \nDavid\n"},{"id":"185161","messageId":"4F44BE03.2000906@gmail.com","threadId":"29677","inReplyTo":"1329785969-828-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-02-22T10:05:55Z","receivedAt":"2012-02-22T10:05:55Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 02/21/2012 01:59 AM, Tim Henigan wrote:\n> test -z $(which mktemp 2>/dev/null)\n>\nThis is wrong: if mktemp is not avilable, the expression above will\nbecome, after command substitution and word splitting have taken pace,\nequivalent to:\n\n  test -z\n\nwhich, per POSIX, must return 0 (and does with at least bash 4.1.5 and\ndash 0.5.5.1).  You should just use this instead:\n\n  which mktemp 2>/dev/null\n\nOK, technically you could also fix your idiom above a little and use:\n\n  test -z \"$(which mktemp 2>/dev/null)\"\n\nbut seems like a useless use of indirections to me.\n\nAnd all of this is naturally render moot by Junio's advice of not using\nwhich(1) in the first place ;-)\n\nRegards,\n  Stefano\n"},{"id":"185191","messageId":"CAFouetgyYRKT7h1h4hv40Kxp=ibq1Hw-1mzFe6DsyAUknXKyYQ@mail.gmail.com","threadId":"29677","inReplyTo":"CAJDDKr72rPqp1z-12Ht3Q2UaeUVFutKwoOgkD1G+SbhsBs+p1A@mail.gmail.com","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-02-22T20:14:43Z","receivedAt":"2012-02-22T20:14:43Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Feb 22, 2012 at 4:15 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Tim Henigan <tim.henigan@gmail.com> writes:\n>>\n>> David, any idea on this?\n>\n> I don't see any bash-isms there myself, either.  We should keep this\n> stuff without bash-isms.\n> I haven't had time to read these patches in depth yet but will try to\n> read the re-roll.\n>\n> Can we ask the github user to elaborate on what exactly was erroring out?\n> Does dash not handle || inside $()?  We can only make wild guesses\n> without their help.\n>\n> The only hint from the pull request is \"silent exit with no results\".\n> Do we do that?\n> There are a few code paths where we do \"exit 1\" but that's only under\n> error conditions.\n>\n> We haven't had any reports about git-mergetool/difftool, which use\n> these functions...\n> are we certain the problem was not some other bash-isms in the script?\n\nI have not heard back from the user, but I tested on Ubuntu earlier\ntoday.  I found that when using an older version of the script\n(fbedb7a in the GitHub repo), I could repeat the error.  It is the\n'git-diffall' script that fails silently, I believe due to a bash-ism\nthat was fixed in a subsequent commit.\n\nIn my latest local version of the script, I have switched back to\n#!/bin/sh.  It runs successfully, so I will squash the change into v2\nof the patch.\n\nFor what it is worth, since that bug was originally reported, I have\nbeen running \"checkbashisms\" [1] on the git-diffall script.  That\nutility reports that the script is clean.\n\n-Tim\n\n[1]: http://sourceforge.net/projects/checkbaskisms/\n"},{"id":"185206","messageId":"7vzkca8to9.fsf@alter.siamese.dyndns.org","threadId":"29677","inReplyTo":"CAFouetgyYRKT7h1h4hv40Kxp=ibq1Hw-1mzFe6DsyAUknXKyYQ@mail.gmail.com","subject":"Re: [PATCH] contrib: added git-diffall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T21:06:14Z","receivedAt":"2012-02-22T21:06:14Z","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> I have not heard back from the user, but I tested on Ubuntu earlier\n> today.  I found that when using an older version of the script\n> (fbedb7a in the GitHub repo), I could repeat the error.  It is the\n> 'git-diffall' script that fails silently, I believe due to a bash-ism\n> that was fixed in a subsequent commit.\n\nThanks.\n\nThat would explain it, especially given that you've been doing:\n\n> For what it is worth, since that bug was originally reported, I have\n> been running \"checkbashisms\" [1] on the git-diffall script.  That\n> utility reports that the script is clean.\n\nThe checkbashisms scritp is interesting ;-)\n\nI do not get much of the comment removal and string munging that happen\nbefore it really start to check the contents for bash-isms, but most of\nthe check logic contained within the %foo_bashisms seem to be looking for\nthe right kind of mistakes people who are too used to bash make very\noften.\n"}]}