Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1

From: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Date: Jun 10, 2015, 15:56 UTC
Message-ID: <58099623.334723.1433951804504.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <vpq8ubrtws0.fsf@anie.imag.fr>

Show 20 quoted lines

> > +        git stripspace --strip-comments |
> > +        while read -r command sha1 rest
> > +        do
> > +                case $command in
> > +                ''|noop|x|"exec")
> > +                        ;;
> > +                pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> > +                        if test -z $sha1
> > +                        then
> > +                                echo "$command $rest" >>"$todo".badsha
> > +                        else
> > +                                sha1_verif="$(git rev-parse --verify --quiet $sha1^{commit})"
> > +                                if test -z $sha1_verif
> > +                                then
> > +                                        echo "$command $sha1 $rest" \
> > +                                                >>"$todo".badsha
> 
> When you reach the right end of your screen because of indentation,
> cutting lines with \ is rarely the best option. Having 5 levels of
> indentation is a sign that you should make more functions.

Yeah, I wasn't overly happy with that either, I will try to add some functions (your example seems like a good way of refactoring).

Show 16 quoted lines

> > +                                commit="$(git rev-list --oneline -1 --ignore-missing $sha1 2>/dev/null)"
> > +                                if test -z "$commit"
> > +                                then
> > +                                        echo "$command $sha1 $rest" \
> > +                                                >>"$todo".badcmd
> > +                                else
> > +                                        echo "$command $commit" >>"$todo".badcmd
> > +                                fi
> > +                        fi
> 
> What are you trying to do here? It seems that you are trying to recover
> the line, but the line is your input, you shouldn't have to recompute
> it.
> 
> Why isn't printf '%s %s %s' "$command" "$sha1" "$rest" sufficient in all
> cases?

It is mainly because here the SHA-1 is a long one (40 chars), however I agree that computing short_sha1 and then printf '%s %s %s' "$command" "$short_sha1" "$rest" should be good in this case.

Show 10 quoted lines

> Maybe it would be better to read line by line (to avoid loosing
> whitespace information for example), like
> 
>         while read -r line
>         do
>                 printf '%s' "$line" | read -r cmd sha1 rest
>                 case $sha1 in
>                         ...
> 
> or maybe it's overkill.

Could be a good idea, though I am not completely convinced about it yet.

Noted for the other points.

Thanks, Rémi