Re: [PATCH v2 3/3] subtree: adjust style to match CodingGuidelines

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2016, 17:11 UTC
Message-ID: <xmqq1t2fxayw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160727005638.9972-1-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 7 quoted lines

> Prefer "test" over "[ ... ]", use double-quotes around variables, break
> long lines, and properly indent "case" statements.
>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This is a replacement patch that addresses the notes from Hannes' review.

Thanks.

Show 13 quoted lines

>  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

Show 15 quoted lines

> @@ -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.

Show 6 quoted lines

>  cache_miss()
>  {
> -	for oldrev in $*; do
> -		if [ ! -r "$cachedir/$oldrev" ]; then
> +	for oldrev in "$@"
> +	do

Ditto.

Show 9 quoted lines

> @@ -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.