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

[Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>]

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

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

> 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