From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 24 Feb 2012 20:03:32 GMT
Subject: Re: [PATCH v3] contrib: added git-diffall
Message-ID: <7vzkc8xaln.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1330044581-792-1-git-send-email-tim.henigan@gmail.com>

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

> The 'git difftool' command allows the user to view diffs using an
> external tool.  It runs a separate instance of the tool for each
> file in the diff. This makes it tedious to review changes spanning
> multiple files.
>
> The 'git-diffall' script instead prepares temporary directories
> with the files to be compared and launches a single instance of
> the external diff tool to view them (i.e. a directory diff).
>
> The 'diff.tool' or 'merge.tool' configuration variable is used
> to specify which external tool is used.

Shouldn't the primary interface "--extcmd=" also be advertised here?

> diff --git a/contrib/diffall/README b/contrib/diffall/README
> new file mode 100644
> index 0000000..111f3f6
> --- /dev/null
> +++ b/contrib/diffall/README
> @@ -0,0 +1,24 @@
> +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.

Same here.

> +This script is compatible with most common 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 commit

I seem to recall the previous round said "named commits" here...

> +  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>*]

Doesn't 3. above make "--" mandatory if you give a pathspec?  I.e.

	... take an optional pathspec [-- <pathspec>]*

> diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
> new file mode 100755
> index 0000000..e00fe89
> --- /dev/null
> +++ b/contrib/diffall/git-diffall
> @@ -0,0 +1,261 @@
> ...
> +# mktemp is not available on all platforms (missing from msysgit)
> +# Use a hard-coded tmp dir if it is not available
> +tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
> +	tmp=/git-diffall-tmp.$$

Missing /tmp in front?

> +	mkdir "$tmp" || exit 1
> +}
> + ...
> +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)
> +		if test -z $2

testing $# would be more logical, no?

> +		then
> +			echo You must specify the tool for use with --extcmd
> +			usage
> +		else
> +			diff_tool=$2
> +			shift
> +		fi
> +		;;
> +	--)
> +		path_sep=1

The name path_sep makes one wonder "are we somehow talking about
differences between '/' vs '\' here?"  In other parts of the system, this
is typically called "dashdash_seen"; the meaning of it is "expect nothing
but pathspecs from here on", so "expect_pathspec" could be an alternative
name for it.

> ...
> +elif test -n "$compare_staged"
> +then
> +	while read name
> +	do
> +		ls_list=$(git ls-files -- "$name")

Ok, the use of $name is now slightly tightened with the surrounding dq
throughout the script.  Better (even though $paths are still prone to
splitting by $IFS).

Thanks, will replace what was queued in 'pu'.