Comments inline, though nothing that I think should prevent it from
being added as an ext/ script.

IOW: +1, with some questions that don't affect the +1

-- 
Jacob Helwig

On Wed, 18 Aug 2010 15:59:52 -0700, Rein Henrichs wrote:
> 
> Example:
> 
>     ./ext/facter-diff 1.5.5 1.5.8rc1
> 
> Signed-off-by: Rein Henrichs <[email protected]>
> ---
>  ext/facter-diff |   73 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 73 insertions(+), 0 deletions(-)
>  create mode 100755 ext/facter-diff
> 
> diff --git a/ext/facter-diff b/ext/facter-diff
> new file mode 100755
> index 0000000..361b80b
> --- /dev/null
> +++ b/ext/facter-diff
> @@ -0,0 +1,73 @@
> +#!/usr/bin/env sh
> +#
> +# Output the difference between a facter command run on two different 
> versions
> +# of facter. Uses unified diff format.
> +
> +OPTIONS_SPEC="\
> +facter-diff [options] <rev1> <rev2> [fact]...
> +
> +Example:
> +
> +    ./ext/facter-diff 1.5.7 1.0.2
> +
> +--
> +h,help      Display this help"
> +
> +. "$(git --exec-path)/git-sh-setup"

Is this actually necessary?  If so, why?

> +eval "$(echo "$OPTIONS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit 
> $?)"
> +trap 'err=$?; cleanup; exit $err' 0
> +
> +cleanup() {
> +  test $origin && git checkout "$origin" &> /dev/null
> +}
> +
> +facter() {
> +  ruby -Ilib bin/facter "$@"
> +}
> +
> +log_facter_run() {
> +  local ref=$1 && shift
> +  local tmpfile=$1 && shift
> +
> +  git checkout -f "$ref" &> /dev/null ||

git checkout -q -f "$ref" || ...  ?

Wouldn't "-q" give you the "&> /dev/null" behavior that you'd want (still
displaying errors if checkout fails).

> +    die "fatal: unable to checkout $ref"
> +  facter "$@" > $tmpfile
> +}
> +
> +verify_revision() {
> +  git rev-parse --verify --quiet "$1" > /dev/null ||
> +    die "fatal: '$1' is not a valid revision"
> +}
> +
> +test $1 = "--" && shift # git rev-parse seems to leave the -- in
> +test $# -eq 0 && usage
> +
> +test $# -gt 1 ||
> +  die "fatal: must specify two revisions"
> +
> +status=$(git status -s)
> +test -z "$status" ||
> +  die "fatal: $0 cannot be used with a dirty working copy"
> +
> +origin=$(git name-rev --name-only HEAD)

This will fail if you're currently in a detached HEAD state.  Might be
worth mentioning in the usage?

% git name-rev --name-only HEAD
master
% git checkout HEAD^0
HEAD is now at 9855b08... Start 1.7.4 cycle
% git name-rev --name-only HEAD
master

I'd end up on master, instead of back in my detached HEAD state (that
just happens to coincide with a ref).

> +test -n "$origin" ||
> +  die "fatal: $0 cannot be used unless currently on a branch"
> +
> +test -x "bin/facter" ||
> +  die "fatal: $0 must be run from the project root"
> +
> +ref1="$1" && shift
> +ref2="$1" && shift
> +
> +verify_revision $ref1
> +verify_revision $ref2
> +
> +tmpfile1="/tmp/$(basename $0).$$.1.tmp"
> +tmpfile2="/tmp/$(basename $0).$$.2.tmp"
> +
> +log_facter_run $ref1 $tmpfile1 $@
> +log_facter_run $ref2 $tmpfile2 $@
> +
> +git checkout -f "$origin" &> /dev/null
> +
> +diff --label "$ref1" --label "$ref2" -u $tmpfile1 $tmpfile2
> -- 
> 1.7.0.4
> 

Attachment: signature.asc
Description: Digital signature

Reply via email to