Re: [PATCH v3] contrib: added git-diffall
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:08
Tim Henigan [off-list ref] 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?
quoted hunk ↗ jump to hunk
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>]*
quoted hunk ↗ jump to hunk
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'.