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

From: Ping Yin <pkufranky@gmail.com>
Date: Jan 12, 2008, 09:48 UTC
Message-ID: <46dff0320801120148r1a760bcdq4b9dcdf31354d9e0@mail.gmail.com>
In-Reply-To: <7vejcnv3zk.fsf@gitster.siamese.dyndns.org>

On Jan 12, 2008 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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

Show 30 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".
>
> > +                     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"?

Show 10 quoted lines

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