2014-05-22 8:49 GMT+02:00 Matthieu Moy <matthieu....@grenoble-inp.fr>:
> Elia Pinto <gitter.spi...@gmail.com> writes:
>
>> This is version 2 of the patch to git-submodule of the
>> patch series "convert test -a/-o to && and ||".
>> It contains the fixes identified by Johannes and
>> Matthieu.
>
> This version of the patch (not the whole series) is
>
> Reviewed-by: Matthieu Moy <matthieu....@imag.fr>
>
> Some comments for next time:
>
> * I agree with Johannes that splitting the series with one patch per
> file is not the right way to split. As a reviewer, I want to
>
> - check that -a trivially translates to &&
> - check that -o trivially translates to ||
> - check non-trivial cases
>
> Interleaving these cases (especially the trivial and non-trivial
> cases) makes the review much harder. For this kind of series, the
> change is trivial, but the review is not (Johannes caught a || Vs &&
> inversion that I didn't find for example, which is quite serious), so
> the "optimize your patches for review" is even more important than
> usual.
>
> * Again, to avoid mixing trivial and non-trivial changes, ...
First of all, thank you. I will take note of your valuable suggestions
for the future.
>
>> @@ -1059,13 +1059,17 @@ cmd_summary() {
>> while read mod_src mod_dst sha1_src sha1_dst status sm_path
>> do
>> # Always show modules deleted or type-changed
>> (blob<->module)
>> - test $status = D -o $status = T && echo "$sm_path" &&
>> continue
>> + case "$status" in
>> + [DT])
>> + printf '%s\n' "$sm_path" &&
>> + continue
>> + esac
>
> turning a echo into a printf is good, but would be better done
> separately.
I had thought, but the change was in the fix of Johannes, and it did
not think was right to change this, especially that it was right
anyway. But I understand very well the observation.
Best Regards
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
