threads / patch / 29948

v2, 5 partscontrib/diffall: improvements

Subject: [PATCH 0/5 v2] contrib/diffall: improvements

## tl;dr

6 messages between Mar 14, 2012 and Mar 14, 2012. Diffs are folded; open one to read it.

replies: 5people: 1as markdown or json

Tim Henigan· Mar 14, 2012, 16:38 UTC · lore

This series implements a number of changes to make the script more robust.

Changes in v2:
  - Fixed a bug in patch 3/5 that copied modified working copy files
    to the wrong tmp directory location. This affected any modified
    working copy file not located at the repo root.
  - Added patch 5/5 to insure that the tmp dir is actually deleted
    when the script is run on Windows.
  - Shortened the commit message summary line for each patch.
Tim Henigan (5):
  contrib/diffall: comment actual reason for 'cdup'
  contrib/diffall: create tmp dirs without mktemp
  contrib/diffall: eliminate use of tar
  contrib/diffall: eliminate duplicate while loops
  contrib/diffall: fix cleanup trap on Windows
 contrib/diffall/git-diffall |   52 ++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 28 deletions(-)
-- 
1.7.10.rc0
Tim Henigan· Mar 14, 2012, 16:38 UTC · re: Tim Henigan · lore

[PATCH 1/5 v2] contrib/diffall: comment actual reason for 'cdup'

The comment from an earlier commit did not reflect the actual reason this operation is needed.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
v2 did not affect this patch.
 contrib/diffall/git-diffall |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to contrib/diffall/git-diffall +3 −1
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
index 9bbd27f..d706a6d 100755
--- a/contrib/diffall/git-diffall
+++ b/contrib/diffall/git-diffall
@@ -36,7 +36,9 @@ fi
 
 start_dir=$(pwd)
 
-# needed to access tar utility
+# All the file paths returned by the diff command are relative to the root
+# of the working copy. So if the script is called from a subdirectory, it
+# must switch to the root of working copy before trying to use those paths.
 cdup=$(git rev-parse --show-cdup) &&
 cd "$cdup" || {
 	echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"

-- 
1.7.10.rc0
Tim Henigan· Mar 14, 2012, 16:38 UTC · re: Tim Henigan · lore

[PATCH 2/5 v2] contrib/diffall: create tmp dirs without mktemp

mktemp is not available on all platforms. Instead of littering the code with a work-around, this commit replaces mktemp with a one-line Perl script.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
v2 did not affect this patch.
 contrib/diffall/git-diffall |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
Show changes to contrib/diffall/git-diffall +4 −7
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
index d706a6d..443f646 100755
--- a/contrib/diffall/git-diffall
+++ b/contrib/diffall/git-diffall
@@ -45,13 +45,10 @@ cd "$cdup" || {
 	exit 1
 }
 
-# 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=/tmp/git-diffall-tmp.$$
-	mkdir "$tmp" || exit 1
-}
-
+# set up temp dir
+tmp=$(perl -e 'use File::Temp qw(tempdir);
+	$t=tempdir("/tmp/git-diffall.XXXXX") or exit(1);
+	print $t') || exit 1
 trap 'rm -rf "$tmp" 2>/dev/null' EXIT
 
 left=

-- 
1.7.10.rc0
Tim Henigan· Mar 14, 2012, 16:38 UTC · re: Tim Henigan · lore

[PATCH 3/5 v2] contrib/diffall: eliminate use of tar

The 'tar' utility is not available on all platforms (some only support 'gnutar'). An earlier commit created a work-around for this problem, but a better solution is to eliminate the use of 'tar' completely.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Changes in v2:
  - Added missing '$name' component to the 'cp' target.  This bug
    resulted in modified working copy files being copied to the wrong
    location in the tmp directory.  It only affected files in sub-
    directories of the repo root.
 contrib/diffall/git-diffall |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to contrib/diffall/git-diffall +8 −4
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
index 443f646..f981ac1 100755
--- a/contrib/diffall/git-diffall
+++ b/contrib/diffall/git-diffall
@@ -202,10 +202,14 @@ then
 		fi
 	done < "$tmp/filelist"
 else
-	# Mac users have gnutar rather than tar
-	(tar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && tar -x)) || {
-		gnutar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && gnutar -x)
-	}
+	while read name
+	do
+		if test -e "$name"
+		then
+			mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+			cp "$name" "$tmp/$right_dir/$name"
+		fi
+	done < "$tmp/filelist"
 fi
 
 # Populate the tmp/left_dir directory with the files to be compared

-- 
1.7.10.rc0
Tim Henigan· Mar 14, 2012, 16:38 UTC · re: Tim Henigan · lore

[PATCH 4/5 v2] contrib/diffall: eliminate duplicate while loops

There were 3 instances of a 'while read; do' that used identical logic to populate '/tmp/right_dir'. This commit groups them into a single loop.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
v2 did not affect this patch.
 contrib/diffall/git-diffall |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)
Show changes to contrib/diffall/git-diffall +9 −15
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
index f981ac1..91a31c8 100755
--- a/contrib/diffall/git-diffall
+++ b/contrib/diffall/git-diffall
@@ -179,38 +179,32 @@ fi
 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
+while read name
+do
+	if test -n "$right"
+	then
 		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
+	elif test -n "$compare_staged"
+	then
 		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
-	while read name
-	do
+	else
 		if test -e "$name"
 		then
 			mkdir -p "$tmp/$right_dir/$(dirname "$name")"
 			cp "$name" "$tmp/$right_dir/$name"
 		fi
-	done < "$tmp/filelist"
-fi
+	fi
+done < "$tmp/filelist"
 
 # Populate the tmp/left_dir directory with the files to be compared
 while read name

-- 
1.7.10.rc0
Tim Henigan· Mar 14, 2012, 16:38 UTC · re: Tim Henigan · lore

[PATCH 5/5 v2] contrib/diffall: fix cleanup trap on Windows

Prior to this commit, the cleanup trap that removes the tmp dir created by the script would fail on Windows. The error was silently ignored by the script.

On Windows, a directory cannot be removed while it is the working directory of the process (thanks to Johannes Sixt on the Git list for this info [1]).

This commit eliminates the 'cd' into the tmp directory that caused the error.

[1]: http://article.gmane.org/gmane.comp.version-control.git/193086
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
This patch was added in v2 of the series.
 contrib/diffall/git-diffall |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to contrib/diffall/git-diffall +3 −4
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
index 91a31c8..84f2b65 100755
--- a/contrib/diffall/git-diffall
+++ b/contrib/diffall/git-diffall
@@ -49,7 +49,7 @@ cd "$cdup" || {
 tmp=$(perl -e 'use File::Temp qw(tempdir);
 	$t=tempdir("/tmp/git-diffall.XXXXX") or exit(1);
 	print $t') || exit 1
-trap 'rm -rf "$tmp" 2>/dev/null' EXIT
+trap 'rm -rf "$tmp"' EXIT
 
 left=
 right=
@@ -233,9 +233,8 @@ do
 	fi
 done < "$tmp/filelist"
 
-cd "$tmp"
-LOCAL="$left_dir"
-REMOTE="$right_dir"
+LOCAL="$tmp/$left_dir"
+REMOTE="$tmp/$right_dir"
 
 if test -n "$diff_tool"
 then

-- 
1.7.10.rc0

← back to recent threads