Re: [PATCH 2/5] git-submodule: New subcommand 'summary' (2) - hard work

[Junio C Hamano <gitster@pobox.com>]

Ping Yin <pkufranky@gmail.com> writes:

> +	git diff $cache_option --raw $head -- $modules |
> +	grep '^:160000\|:000000 160000' |
> +	cut -c2- |
> +	while read mod_src mod_dst sha1_src sha1_dst status name
> +	do
> +		sha1_dst=$(echo $sha1_dst | cut -c1-7)
> +		sha1_src=$(echo $sha1_src | cut -c1-7)

If you are willing to lose precision forever like this, I think
you can run "git diff" with --abbrev and lose this cut.

> +		check_dst=t
> +		check_src=t
> +		case $status in
> +		D)
> +			check_dst=
> +			;;
> +		A)
> +			check_src=
> +			;;

I'd loosen the above grep (see my comments to your 1/5) and also
add this:

		*)
			continue ;# punt
			;;

so that the rest of the code won't break when seeing a path that
was submodule in the HEAD but is a blob in the index.

> +		esac
> +
> +		(
> +			errmsg=
> +			unfound_src=
> +			unfound_dst=
> +
> +			test -z "$check_src" ||
> +			GIT_DIR="$name/.git" git-rev-parse $sha1_src >&/dev/null ||

And the precision of $sha1_src matter here.  Be it done with
"diff --abbrev" at the toplevel or your "cut", that may not be
unique enough in the submodule.

I think you would want to read full 40-char sha1_src and
sha1_dst with "while read", and keep that full 40-char in these
variables, and use them when calling rev-parse here.

If you are checking if that the object exists in the submodule,
use "rev-parse --verify", which was designed for exactly that
purpose.  If you also want to verify if the object is a commit,
which may be a good idea anyway, "rev-parse --verify $sha1_src^0".

To be portable, use traditional ">/dev/null 2>&1", not ">&/dev/null".

> +			case "$unfound_src,$unfound_dst" in
> +			t,)
> +				errmsg="  Warn: $name doesn't contain commit $sha1_src"
> +				;;
> +			,t)
> +				errmsg="  Warn: $name doesn't contain commit $sha1_dst"
> +				;;
> +			t,t)
> +				errmsg="  Warn: $name doesn't contain commits $sha1_src and $sha1_dst"
> +				;;

When reporting errors, you would want to give full 40-chars...

> +			*)
> +				left=
> +				right=
> +				test -n "$check_src" &&
> +				left=$(GIT_DIR="$name/.git" git log --pretty=format:"  <%s" \
> +				${check_dst:+$sha1_dst..}$sha1_src 2>/dev/null)
> +
> +				test -n "$check_dst" &&
> +				right=$(GIT_DIR="$name/.git" git log --reverse --pretty=format:"  >%s" \
> +				${check_src:+$sha1_src..}$sha1_dst 2>/dev/null)
> +				;;
> +			esac
> +
> +			echo "* $name $sha1_src...$sha1_dst:"

While reporting like this, you would want the shortened form,
perhaps produced your "cut -c1-7".

> +			if test -n "$errmsg"
> +			then
> +				echo "$errmsg"
> +			else
> +				test -n "$left" && echo "$left"
> +				test -n "$right" && echo "$right"
> +			fi
> +			echo
> +		) | sed 's/^/# /'
> +	done

I'd prefer to always have "-e" before the sed expression.

Any reason why you want separate invocation of sed inside the
while loop?  IOW, why isn't it like this?

	git diff --raw |
        while read ...
        do
        	...
	done | sed -e 's/^/# /'

> +
>  	cd "$cwd"

Hmmm.