Elia Pinto <gitter.spiros@xxxxxxxxx> 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.Moy@xxxxxxx>
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, ...
> @@ -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.
--
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html