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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 12, 2008, 08:32 UTC
Message-ID: <7vejcnv3zk.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <62a73e734832ad67e89be706f1f8b3dbc30cfcf4.1200122041.git.pkufranky@gmail.com>

Ping Yin <pkufranky@gmail.com> writes:

Show 7 quoted lines

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

Show 9 quoted lines

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

Show 9 quoted lines

> +		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".

Show 10 quoted lines

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

Show 14 quoted lines

> +			*)
> +				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".

Show 10 quoted lines

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