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

["Ping Yin" <pkufranky@gmail.com>]

On Jan 12, 2008 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>
> > +             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) .

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

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