David Aguilar <dav...@gmail.com> writes:
> Prefer "test" over "[ ... ]", use double-quotes around variables, break
> long lines, and properly indent "case" statements.
>
> Helped-by: Johannes Sixt <j...@kdbg.org>
> Signed-off-by: David Aguilar <dav...@gmail.com>
> ---
> This is a replacement patch that addresses the notes from Hannes' review.
Thanks.
> case "$command" in
> - add) [ -e "$prefix" ] &&
> - die "prefix '$prefix' already exists." ;;
> - *) [ -e "$prefix" ] ||
> - die "'$prefix' does not exist; use 'git subtree add'" ;;
> +add)
> + test -e "$prefix" && die "prefix '$prefix' already exists."
> + ;;
> +*)
> + test -e "$prefix" ||
> + die "'$prefix' does not exist; use 'git subtree add'"
> + ;;
> esac
Comparing the above with the below
> @@ -145,16 +204,21 @@ debug
> cache_setup()
> {
> cachedir="$GIT_DIR/subtree-cache/$$"
> - rm -rf "$cachedir" || die "Can't delete old cachedir: $cachedir"
> - mkdir -p "$cachedir" || die "Can't create new cachedir: $cachedir"
> - mkdir -p "$cachedir/notree" || die "Can't create new cachedir:
> $cachedir/notree"
> + rm -rf "$cachedir" ||
> + die "Can't delete old cachedir: $cachedir"
> + mkdir -p "$cachedir" ||
> + die "Can't create new cachedir: $cachedir"
> + mkdir -p "$cachedir/notree" ||
> + die "Can't create new cachedir: $cachedir/notree"
> debug "Using cachedir: $cachedir" >&2
> }
makes me think the former would look more readble if consistently
formatted line this (note: I untabified to keep the alignment, so
please do not cut and paste from here):
case "$command" in
add)
test -e "$prefix" &&
die "prefix '$prefix' already exists."
;;
*)
test -e "$prefix" ||
die "'$prefix' does not exist; use 'git subtree add'"
;;
esac
> cache_get()
> {
I noticed this in the CodingGuidelines:
- We prefer a space between the function name and the parentheses,
and no space inside the parentheses. The opening "{" should also
be on the same line.
perhaps do it as well while we are at it?
> - for oldrev in $*; do
> - if [ -r "$cachedir/$oldrev" ]; then
> + for oldrev in "$@"
> + do
This looked fishy, but all the callers of cache_get are doing the
right thing. They either throw a single token ("latest_new",
"latest_old") at it, or give "$rev" (dquoted) or $parents (not
dquoted) that is taken by reading the "rev-list --parents" output
with "read rev parents" one line at a time.
> cache_miss()
> {
> - for oldrev in $*; do
> - if [ ! -r "$cachedir/$oldrev" ]; then
> + for oldrev in "$@"
> + do
Ditto.
> @@ -172,9 +238,11 @@ cache_miss()
>
> check_parents()
> {
> - missed=$(cache_miss $*)
> - for miss in $missed; do
> - if [ ! -r "$cachedir/notree/$miss" ]; then
> + missed=$(cache_miss "$@")
> + for miss in $missed
Ditto.
There is an obvious "optimization potential" here in that $missed is
otherwise never used, but it is outside the scope of the stylefix
patch.
> + if test -n "$old"
> + then
> + squash_msg "$dir" "$oldsub" "$newsub" |
> git commit-tree "$tree" -p "$old" || exit $?
The change to the third line is to remove the trailing SP, which is
welcome. While we are touching it up, "git commit-tree" should be
dedented to align with "squash_msg" (the same in the else clause)
to be consistent with the remainder of the script (and the system),
I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
