From: Junio C Hamano <gitster@pobox•com>
To: Tim Henigan <tim.henigan@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] contrib: added git-diffall
Date: Tue, 21 Feb 2012 13:51:39 -0800 [thread overview]
Message-ID: <7vd397g8ic.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1329785969-828-1-git-send-email-tim.henigan@gmail.com> (Tim Henigan's message of "Mon, 20 Feb 2012 19:59:29 -0500")
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.
next prev parent reply other threads:[~2012-02-21 21:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-21 0:59 [PATCH] contrib: added git-diffall Tim Henigan
2012-02-21 21:51 ` Junio C Hamano [this message]
2012-02-22 2:02 ` Tim Henigan
2012-02-22 2:41 ` Junio C Hamano
2012-02-22 9:15 ` David Aguilar
2012-02-22 20:14 ` Tim Henigan
2012-02-22 21:06 ` Junio C Hamano
2012-02-22 10:05 ` Stefano Lattarini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vd397g8ic.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tim.henigan@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox