Re: [PATCH v2 3/3] subtree: adjust style to match CodingGuidelines
From: Junio C Hamano <hidden>
Date: 2016-07-27 17:11:28
David Aguilar [off-list ref] writes:
Prefer "test" over "[ ... ]", use double-quotes around variables, break long lines, and properly indent "case" statements. Helped-by: Johannes Sixt [off-list ref] Signed-off-by: David Aguilar <redacted> --- 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
quoted hunk
@@ -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 "$@"
+ doDitto.
quoted hunk
@@ -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.