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

Ping Yin [off-list ref] 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.

On Jan 12, 2008 4:32 PM, Junio C Hamano [off-list ref] wrote:

Ping Yin [off-list ref] writes:

quoted

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

Right. I thought 'M' should be the default case because I missed the
case 'T' (head blob but index submodule, or the reverse) .

quoted

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

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.

Hmm, precision is really a problem. However, "git diff --raw" will not
always give full 40-char sha1, instead it will give sha1 with enough
length. So maybe i can use the sha1 from "git diff --raw" ?

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

Yes, --verify is better.
                   ;;

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

As said before, enough is ok?

quoted

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

quoted

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

Is it not portable without "-e"?

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/^/# /'

Just because i'm stupid. :)



-- 
Ping Yin

quoted

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.

Hmm, precision is really a problem. However, "git diff --raw" will not
always give full 40-char sha1, instead it will give sha1 with enough
length. So maybe i can use the sha1 from "git diff --raw" ?

Oh, I'm wrong. It seems 'git diff --raw' will always give full 40-char
sha1 for submodule entry and abbreviated sha1 for blob entry.

-- 
Ping Yin