[PATCH] contrib: added git-diffall

[PATCH] contrib: added git-diffall

[Tim Henigan]

This script adds directory diff support to git.  It launches a single
instance of the user-configured external diff tool and performs a
directory diff between the specified revisions. The before/after files
are copied to a tmp directory to do this.  Either 'diff.tool' or
'merge.tool' must be set before running the script.

The existing 'git difftool' command already allows the user to view diffs
using an external tool.  However if multiple files have changed, a
separate instance of the tool is launched for each one.  This can be
tedious when many files are involved.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This script has been hosted on GitHub [1] since April 2010. Enough people
have found it useful that I hope it will be considered for inclusion in
the standard git install, either in contrib or as a new core command.

[1]: https://github.com/thenigan/git-diffall

 contrib/diffall/README      |   18 +++
 contrib/diffall/git-diffall |  275 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 293 insertions(+)
 create mode 100644 contrib/diffall/README
 create mode 100755 contrib/diffall/git-diffall

diff --git a/contrib/diffall/README b/contrib/diffall/README
new file mode 100644
index 0000000..12881d2
--- /dev/null
+++ b/contrib/diffall/README
@@ -0,0 +1,18 @@
+The git-diffall script provides a directory based diff mechanism
+for git.  The script relies on the diff.tool configuration option
+to determine what diff viewer is used.
+
+This script is compatible with all the forms used to specify a
+range of revisions to diff:
+
+  1. git diffall: shows diff between working tree and staged changes
+  2. git diffall --cached [<commit>]: shows diff between staged changes and HEAD (or other named commit)
+  3. git diffall <commit>: shows diff between working tree and named commit
+  4. git diffall <commit> <commit>: show diff between two named commits
+  5. git diffall <commit>..<commit>: same as above
+  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>
+
+Note: all forms take an optional path limiter [--] [<path>*]
+
+This script is based on an example provided by Thomas Rast on the Git list [1]:
+[1] http://thread.gmane.org/gmane.comp.version-control.git/124807
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
new file mode 100755
index 0000000..ef01eda
--- /dev/null
+++ b/contrib/diffall/git-diffall
@@ -0,0 +1,275 @@
+#!/bin/bash -e
+# Copyright 2010 - 2012, Tim Henigan <tim.henigan@gmail.com>
+#
+# Perform a directory diff between commits in the repository using
+# the external diff or merge tool specified in the user's config.
+
+USAGE='[--cached] [--copy-back] [-x|--extcmd=<command>] <commit>{0,2} -- <path>*
+
+    --cached     Compare to the index rather than the working tree.
+
+    --copy-back  Copy files back to the working tree when the diff
+                 tool exits (in case they were modified by the
+                 user).  This option is only valid if the diff
+                 compared with the working tree.
+
+    -x=<command>
+    --extcmd=<command>  Specify a custom command for viewing diffs.
+                 git-diffall ignores the configured defaults and
+                 runs $command $LOCAL $REMOTE when this option is
+                 specified. Additionally, $BASE is set in the
+                 environment.
+'
+
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+TOOL_MODE=diff
+. "$(git --exec-path)/git-mergetool--lib"
+
+merge_tool="$(get_merge_tool)"
+if test -z "$merge_tool"
+then
+	echo "Error: Either the 'diff.tool' or 'merge.tool' option must be set."
+	usage
+fi
+
+start_dir=$(pwd)
+
+# needed to access tar utility
+cdup=$(git rev-parse --show-cdup) &&
+cd "$cdup" || {
+	echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+	exit 1
+}
+
+# mktemp is not available on all platforms (missing from msysgit)
+# Use a hard-coded tmp dir if it is not available
+if test -z $(which mktemp 2>/dev/null)
+then
+	tmp=/tmp/git-diffall-tmp
+else
+	tmp="$(mktemp -d -t tmp.XXXXXX)"
+fi
+mkdir -p "$tmp"
+
+left=
+right=
+paths=
+path_sep=
+compare_staged=
+common_ancestor=
+left_dir=
+right_dir=
+diff_tool=
+copy_back=
+
+while test $# != 0
+do
+	case "$1" in
+	-h|--h|--he|--hel|--help)
+		usage
+		;;
+	--cached)
+		compare_staged=1
+		;;
+	--copy-back)
+		copy_back=1
+		;;
+	-x|--e|--ex|--ext|--extc|--extcm|--extcmd)
+		diff_tool=$2
+		shift
+		;;
+	--)
+		path_sep=1
+		;;
+	-*)
+		echo Invalid option: "$1"
+		usage
+		;;
+	*)
+		# could be commit, commit range or path limiter
+		case "$1" in
+		*...*)
+			left=${1%...*}
+			right=${1#*...}
+			common_ancestor=1
+			;;
+		*..*)
+			left=${1%..*}
+			right=${1#*..}
+			;;
+		*)
+			if test -n "$path_sep"
+			then
+				if test -z "$paths"
+				then
+					paths=$1
+				else
+					paths="$paths $1"
+				fi
+			elif test -z "$left"
+			then
+				left=$1
+			elif test -z "$right"
+			then
+				right=$1
+			else
+				if test -z "$paths"
+				then
+					paths=$1
+				else
+					paths="$paths $1"
+				fi
+			fi
+			;;
+		esac
+		;;
+	esac
+	shift
+done
+
+# Determine the set of files which changed
+if test -n "$left" && test -n "$right"
+then
+	left_dir="cmt-$(git rev-parse --short $left)"
+	right_dir="cmt-$(git rev-parse --short $right)"
+
+	if test -n "$compare_staged"
+	then
+		usage
+	elif test -n "$common_ancestor"
+	then
+		git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
+	else
+		git diff --name-only "$left" "$right" -- $paths > "$tmp"/filelist
+	fi
+elif test -n "$left"
+then
+	left_dir="cmt-$(git rev-parse --short $left)"
+
+	if test -n "$compare_staged"
+	then
+		right_dir="staged"
+		git diff --name-only --cached "$left" -- $paths > "$tmp"/filelist
+	else
+		right_dir="working_tree"
+		git diff --name-only "$left" -- $paths > "$tmp"/filelist
+	fi
+else
+	left_dir="HEAD"
+
+	if test -n "$compare_staged"
+	then
+		right_dir="staged"
+		git diff --name-only --cached -- $paths > "$tmp"/filelist
+	else
+		right_dir="working_tree"
+		git diff --name-only -- $paths > "$tmp"/filelist
+	fi
+fi
+
+# Exit immediately if there are no diffs
+if test ! -s "$tmp"/filelist
+then
+	exit 0
+fi
+
+if test -n "$copy_back" && test "$right_dir" != "working_tree"
+then
+	echo "--copy-back is only valid when diff includes the working tree."
+	exit 1
+fi
+
+# Create the named tmp directories that will hold the files to be compared
+mkdir -p "$tmp"/"$left_dir" "$tmp"/"$right_dir"
+
+# Populate the tmp/right_dir directory with the files to be compared
+if test -n "$right"
+then
+	while read name
+	do
+		ls_list=$(git ls-tree $right $name)
+		if test -n "$ls_list"
+	then
+			mkdir -p "$tmp"/"$right_dir"/"$(dirname "$name")"
+			git show "$right":"$name" > "$tmp"/"$right_dir"/"$name" || true
+		fi
+	done < "$tmp"/filelist
+elif test -n "$compare_staged"
+then
+	while read name
+	do
+		ls_list=$(git ls-files -- $name)
+		if test -n "$ls_list"
+	then
+			mkdir -p "$tmp"/"$right_dir"/"$(dirname "$name")"
+			git show :"$name" > "$tmp"/"$right_dir"/"$name"
+		fi
+	done < "$tmp"/filelist
+else
+	if test -n "$(which gnutar 2>/dev/null)"
+	then
+		gnutar --ignore-failed-read -c -T "$tmp"/filelist | (cd "$tmp"/"$right_dir" && gnutar -x)
+	else
+		tar --ignore-failed-read -c -T "$tmp"/filelist | (cd "$tmp"/"$right_dir" && tar -x)
+	fi
+fi
+
+# Populate the tmp/left_dir directory with the files to be compared
+while read name
+do
+	if test -n "$left"
+	then
+		ls_list=$(git ls-tree $left $name)
+		if test -n "$ls_list"
+		then
+			mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
+			git show "$left":"$name" > "$tmp"/"$left_dir"/"$name" || true
+		fi
+	else
+		if test -n "$compare_staged"
+		then
+			ls_list=$(git ls-tree HEAD $name)
+			if test -n "$ls_list"
+			then
+				mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
+				git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
+				fi
+			else
+				mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
+				git show :"$name" > "$tmp"/"$left_dir"/"$name"
+		fi
+	fi
+done < "$tmp"/filelist
+
+cd "$tmp"
+LOCAL="$left_dir"
+REMOTE="$right_dir"
+
+if test -n "$diff_tool"
+then
+	export BASE
+	eval $diff_tool '"$LOCAL"' '"$REMOTE"'
+else
+	run_merge_tool "$merge_tool" false
+fi
+
+# This function is called on exit
+cleanup () {
+	cd "$start_dir"
+
+	# Copy files back to the working dir, if requested
+	if test -n "$copy_back" && test "$right_dir" = "working_tree"
+	then
+		git_top_dir=$(git rev-parse --show-toplevel)
+		find "$tmp/$right_dir" -type f|while read file; do
+			cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
+		done
+	fi
+
+	# Remove the tmp directory
+	rm -rf "$tmp"
+}
+
+trap cleanup EXIT
-- 
1.7.9.GIT

Re: [PATCH] contrib: added git-diffall

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
> new file mode 100755
> index 0000000..ef01eda
> --- /dev/null
> +++ b/contrib/diffall/git-diffall
> @@ -0,0 +1,275 @@
> +#!/bin/bash -e

Does this have to be bash-only (iow, infested with bash-isms beyond
repair), or is you wrote this merely from inertia?

The following is only after a cursory scanning, so there may be other
things that needs fixing, but anyway:

 - Don't use "which" in scripts.  Its output is not machine parseable, and
   exit code is not reliable, across platforms.  It is only meant for
   consumption by human who can read English (or natural language in the
   current locale).

> +				if test -z "$paths"
> +				then
> +					paths=$1
> +				else
> +					paths="$paths $1"
> +				fi

Just a style tip; if you are going to let shell $IFS split this list
anyway, it is customary to write the above as

	paths="$paths$1 "

> +		git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist

	git diff ... -- $paths >"$tmp/filelist"

> +# Exit immediately if there are no diffs
> +if test ! -s "$tmp"/filelist
> +then
> +	exit 0
> +fi
> +
> +if test -n "$copy_back" && test "$right_dir" != "working_tree"
> +then
> +	echo "--copy-back is only valid when diff includes the working tree."
> +	exit 1
> +fi
> +
> +# Create the named tmp directories that will hold the files to be compared
> +mkdir -p "$tmp"/"$left_dir" "$tmp"/"$right_dir"

	mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"

> +		if test -n "$compare_staged"
> +		then
> +			ls_list=$(git ls-tree HEAD $name)
> +			if test -n "$ls_list"
> +			then
> +				mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> +				git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
> +				fi
> +			else
> +				mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> +				git show :"$name" > "$tmp"/"$left_dir"/"$name"
> +		fi

That's misleadingly indented.  First I thought "in what case would we want
to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
but the overindented four lines starting from the funny "fi" is about non
cached case.

> +	fi
> +done < "$tmp"/filelist
> +
> +cd "$tmp"
> +LOCAL="$left_dir"
> +REMOTE="$right_dir"
> +
> +if test -n "$diff_tool"
> +then
> +	export BASE
> +	eval $diff_tool '"$LOCAL"' '"$REMOTE"'
> +else
> +	run_merge_tool "$merge_tool" false
> +fi
> +
> +# This function is called on exit
> +cleanup () {
> +	cd "$start_dir"
> +
> +	# Copy files back to the working dir, if requested
> +	if test -n "$copy_back" && test "$right_dir" = "working_tree"
> +	then
> +		git_top_dir=$(git rev-parse --show-toplevel)
> +		find "$tmp/$right_dir" -type f|while read file; do
> +			cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
> +		done

Why is this loop written in such a dense way?  Everything else (except for
that misindented part) were almost to our CodingStyle and was fairly easy
to read, though.

> +	fi
> +
> +	# Remove the tmp directory
> +	rm -rf "$tmp"
> +}
> +
> +trap cleanup EXIT

Does this even trigger?  This is not Perl that parses and runs set-up code
before executing everything else, so I suspect this last line amounts to
the same thing as writing just

	cleanup

without trap nor signal names.

If you are to set up temporary files or directories that you want to clean
up, a good discipline is to follow this order:

  - define variable(s) to hold the temporary locations, e.g.
    tmpdir=$(mktemp ...)

  - set the trap before starting to use these temporary locations, e.g.
    trap 'rm -rf "$tmpdir' 0 1 2 3 15

  - and then start populating tmpdir and do whatever you want to do.

Re: [PATCH] contrib: added git-diffall

[Tim Henigan]

Thank you for taking the time to review this patch.  I appreciate it.


On Tue, Feb 21, 2012 at 4:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:

>> +#!/bin/bash -e
>
> Does this have to be bash-only (iow, infested with bash-isms beyond
> repair), or is you wrote this merely from inertia?

There is no specific reason it must be bash.  I changed from
"#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
Ubuntu [1].  The user reported that:

    "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
    This causes git_merge_tool_path to fail. The error isn't trapped,
so it exits
    without displaying anything and without cleaning up."

Given that all the other scripts distributed with git use /bin/sh, I
will change this script to match.


> The following is only after a cursory scanning, so there may be other
> things that needs fixing, but anyway:
>
>  - Don't use "which" in scripts.  Its output is not machine parseable, and
>   exit code is not reliable, across platforms.  It is only meant for
>   consumption by human who can read English (or natural language in the
>   current locale).

I used "which" in two places.  Both were added to support problems
with missing standard tools on certain platforms (missing mktemp on
msysgit and missing option from tar on Mac [2]).  Is there some other
standard way to detect the platform or if certain utils are present?

>> +                             if test -z "$paths"
>> +                             then
>> +                                     paths=$1
>> +                             else
>> +                                     paths="$paths $1"
>> +                             fi
>
> Just a style tip; if you are going to let shell $IFS split this list
> anyway, it is customary to write the above as
>
>        paths="$paths$1 "

Nice...I will change to use this format.


>> +             git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
>
>        git diff ... -- $paths >"$tmp/filelist"
... <snip>
>        mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"

Will change all instances.


>> +             if test -n "$compare_staged"
>> +             then
>> +                     ls_list=$(git ls-tree HEAD $name)
>> +                     if test -n "$ls_list"
>> +                     then
>> +                             mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> +                             git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
>> +                             fi
>> +                     else
>> +                             mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> +                             git show :"$name" > "$tmp"/"$left_dir"/"$name"
>> +             fi
>
> That's misleadingly indented.  First I thought "in what case would we want
> to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
> but the overindented four lines starting from the funny "fi" is about non
> cached case.

That is embarrassing.  I will fix it.

<snip>

>> +             find "$tmp/$right_dir" -type f|while read file; do
>> +                     cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
>> +             done
>
> Why is this loop written in such a dense way?  Everything else (except for
> that misindented part) were almost to our CodingStyle and was fairly easy
> to read, though.

I missed this in my style cleanup.  I will fix it.


>> +     fi
>> +
>> +     # Remove the tmp directory
>> +     rm -rf "$tmp"
>> +}
>> +
>> +trap cleanup EXIT
>
> Does this even trigger?  This is not Perl that parses and runs set-up code
> before executing everything else, so I suspect this last line amounts to
> the same thing as writing just
>
>        cleanup
>
> without trap nor signal names.

The cleanup triggers on all the platforms I have tested (Ubuntu,
msysgit, Mac).  I could change it, but for me it has "just worked".


> If you are to set up temporary files or directories that you want to clean
> up, a good discipline is to follow this order:
>
>  - define variable(s) to hold the temporary locations, e.g.
>    tmpdir=$(mktemp ...)
>
>  - set the trap before starting to use these temporary locations, e.g.
>    trap 'rm -rf "$tmpdir' 0 1 2 3 15
>
>  - and then start populating tmpdir and do whatever you want to do.

I will review the changes needed for this before submitting v2 of my patch.

[1]: https://github.com/thenigan/git-diffall/pull/9
[2]: https://github.com/thenigan/git-diffall/pull/2#issuecomment-498472

Re: [PATCH] contrib: added git-diffall

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> There is no specific reason it must be bash.  I changed from
> "#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
> Ubuntu [1].  The user reported that:
>
>     "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
>     This causes git_merge_tool_path to fail. The error isn't trapped,
> so it exits
>     without displaying anything and without cleaning up."
>
> Given that all the other scripts distributed with git use /bin/sh, I
> will change this script to match.

You need to dig back to that bug report deeper and find out what exactly
is causing the "failure", before blindly allowing /bin/dash to be used.

I think the above function name is a typo of get_merge_tool_path that is
borrowed from git-mergetool--lib.sh, but nothing in the function jumps as
a blatant bash-ism at me from a quick reading.

David, any idea on this?

>> The following is only after a cursory scanning, so there may be other
>> things that needs fixing, but anyway:
>>
>>  - Don't use "which" in scripts.  Its output is not machine parseable, and
>>   exit code is not reliable, across platforms.  It is only meant for
>>   consumption by human who can read English (or natural language in the
>>   current locale).
>
> I used "which" in two places.  Both were added to support problems
> with missing standard tools on certain platforms (missing mktemp on
> msysgit and missing option from tar on Mac [2]).  Is there some other
> standard way to detect the platform or if certain utils are present?

There are examples in the script you are borrowing functions from, even in
the function that allegedly fail for the dash user ;-).

> The cleanup triggers on all the platforms I have tested (Ubuntu,
> msysgit, Mac).  I could change it, but for me it has "just worked".

You set "trap cleanup" for exit event, and then the control reaches the
end of the script, which is an exit event, and the cleanup function is
called.  So it is natural that if you manage to get to that "trap cleanup"
line, of course cleanup will run.  But if you dropped "trap" and "EXIT"
from that line, it amounts to the same thing.

A more important thing to know is that until the control reaches that
"trap cleanup EXIT" line, you do not have that trap set.  So if you caused
the program to exit in an earlier part of the program (say, in the big
"while read name" loop) before the control reaches this line, you won't
see any cleanup.

Perhaps that is what you wanted, but then again writing "trap" and "EXIT"
there is pointless.

Usually people set up a clean-up task with trap before going into complex
stuff, so that no matter where in the complex code the trapped condition
(typically a signal, but an exit event is also possible) happens, they can
be sure the clean-up task is run.  That is the reason behind the following
ordering.

>> If you are to set up temporary files or directories that you want to clean
>> up, a good discipline is to follow this order:
>>
>>  - define variable(s) to hold the temporary locations, e.g.
>>    tmpdir=$(mktemp ...)
>>
>>  - set the trap before starting to use these temporary locations, e.g.
>>    trap 'rm -rf "$tmpdir' 0 1 2 3 15
>>
>>  - and then start populating tmpdir and do whatever you want to do.

Re: [PATCH] contrib: added git-diffall

[David Aguilar]

On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> There is no specific reason it must be bash.  I changed from
>> "#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
>> Ubuntu [1].  The user reported that:
>>
>>     "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
>>     This causes git_merge_tool_path to fail. The error isn't trapped,
>> so it exits
>>     without displaying anything and without cleaning up."
>>
>> Given that all the other scripts distributed with git use /bin/sh, I
>> will change this script to match.
>
> You need to dig back to that bug report deeper and find out what exactly
> is causing the "failure", before blindly allowing /bin/dash to be used.
>
> I think the above function name is a typo of get_merge_tool_path that is
> borrowed from git-mergetool--lib.sh, but nothing in the function jumps as
> a blatant bash-ism at me from a quick reading.
>
> David, any idea on this?

I don't see any bash-isms there myself, either.  We should keep this
stuff without bash-isms.
I haven't had time to read these patches in depth yet but will try to
read the re-roll.

Can we ask the github user to elaborate on what exactly was erroring out?
Does dash not handle || inside $()?  We can only make wild guesses
without their help.

The only hint from the pull request is "silent exit with no results".
Do we do that?
There are a few code paths where we do "exit 1" but that's only under
error conditions.

We haven't had any reports about git-mergetool/difftool, which use
these functions...
are we certain the problem was not some other bash-isms in the script?
-- 
David

Re: [PATCH] contrib: added git-diffall

[Tim Henigan]

On Wed, Feb 22, 2012 at 4:15 AM, David Aguilar <davvid@gmail.com> wrote:
> On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Tim Henigan <tim.henigan@gmail.com> writes:
>>
>> David, any idea on this?
>
> I don't see any bash-isms there myself, either.  We should keep this
> stuff without bash-isms.
> I haven't had time to read these patches in depth yet but will try to
> read the re-roll.
>
> Can we ask the github user to elaborate on what exactly was erroring out?
> Does dash not handle || inside $()?  We can only make wild guesses
> without their help.
>
> The only hint from the pull request is "silent exit with no results".
> Do we do that?
> There are a few code paths where we do "exit 1" but that's only under
> error conditions.
>
> We haven't had any reports about git-mergetool/difftool, which use
> these functions...
> are we certain the problem was not some other bash-isms in the script?

I have not heard back from the user, but I tested on Ubuntu earlier
today.  I found that when using an older version of the script
(fbedb7a in the GitHub repo), I could repeat the error.  It is the
'git-diffall' script that fails silently, I believe due to a bash-ism
that was fixed in a subsequent commit.

In my latest local version of the script, I have switched back to
#!/bin/sh.  It runs successfully, so I will squash the change into v2
of the patch.

For what it is worth, since that bug was originally reported, I have
been running "checkbashisms" [1] on the git-diffall script.  That
utility reports that the script is clean.

-Tim

[1]: http://sourceforge.net/projects/checkbaskisms/

Re: [PATCH] contrib: added git-diffall

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> I have not heard back from the user, but I tested on Ubuntu earlier
> today.  I found that when using an older version of the script
> (fbedb7a in the GitHub repo), I could repeat the error.  It is the
> 'git-diffall' script that fails silently, I believe due to a bash-ism
> that was fixed in a subsequent commit.

Thanks.

That would explain it, especially given that you've been doing:

> For what it is worth, since that bug was originally reported, I have
> been running "checkbashisms" [1] on the git-diffall script.  That
> utility reports that the script is clean.

The checkbashisms scritp is interesting ;-)

I do not get much of the comment removal and string munging that happen
before it really start to check the contents for bash-isms, but most of
the check logic contained within the %foo_bashisms seem to be looking for
the right kind of mistakes people who are too used to bash make very
often.

Re: [PATCH] contrib: added git-diffall

[Stefano Lattarini]

On 02/21/2012 01:59 AM, Tim Henigan wrote:
> test -z $(which mktemp 2>/dev/null)
>
This is wrong: if mktemp is not avilable, the expression above will
become, after command substitution and word splitting have taken pace,
equivalent to:

  test -z

which, per POSIX, must return 0 (and does with at least bash 4.1.5 and
dash 0.5.5.1).  You should just use this instead:

  which mktemp 2>/dev/null

OK, technically you could also fix your idiom above a little and use:

  test -z "$(which mktemp 2>/dev/null)"

but seems like a useless use of indirections to me.

And all of this is naturally render moot by Junio's advice of not using
which(1) in the first place ;-)

Regards,
  Stefano