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

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

Junio C Hamano <gitster@pobox.com> writes:
> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> >  I used:
> >    read -r command sha1 rest <<EOF
> >    $line
> >    EOF
> >  because
> >    printf '%s' "$line" | read -r command sha1 rest
> >  doesn't work (the 3 variables have no value as a result).
> >  There might be a better way to do this, but I don't have it right now.
> 
>         while read line
>         do
>                 (
>                         IFS=' '
>                         set x $line
>                         shift
>                         # now $1 is your command, $2 is sha1, $3 is remainder
>                         ...
>                 )
>         done
> 
> perhaps?

Will try, thanks!

> But more importantly, why do you even need to keep the bad ones in a
> separate .badcmd and .badsha files?  Isn't that bloating your changes
> unnecessarily, iow, if you issued your warning as you encounter them,
> wouldn't the change become cleaner and easier to understand (and as
> a side effect it may even become smaller)?  The _only_ thing that
> you would get by keeping them in temporary files is that you can do
> "one header and bunch of errors", but is it so common to make a bad
> edit to the insn sheet that "a sequence of errors, one per line"
> becomes more burdensome to the end user?
> 
> I would think
> 
>         stripspace |
>         while read -r command sha1 rest
>         do
>                 ...
> 
> and showing the warning as you detect inside that loop would be
> sufficient.  Perhaps I am missing subtle details of what you are
> doing.

You're not missing subtle details, it is as you said, I tough it would
be clearer for the user to have "one header and a bunch of errors".
Moreover while it would make the patch smaller and easier to
understand, I am not sure about making it cleaner; I guess I will have
to try and see how it ends up.

What I'm not completely happy with your proposition is the fact that 
if there are multiple errors of the same kind, the output would look 
something like:
> Warning: the command isn't recognized in the following line:
> badcmd1 some_sha some_commit_message
> Warning: the command isn't recognized in the following line:
> badcmd2 some_sha some_commit_message
(I don't think it would be good to squash some understandable warning
message and the faulty line in one line, it would probably end up
being too long)
However as you say, such mistakes are uncommon so I guess it's fine.

Thanks,
Rémi