On Tue, Sep 28, 2010 at 09:23:57AM -0400, Jeff King wrote: > On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote: > > > > Granted, echo is forgiving and will concatenate the arguments it gets with > > > a space in between, but you would either want to either: > > > > > > (1) make it more explicit that helper gets only one argument, by saying > > > "$1" instead of "$@", in all places in the helper script; or > > > > > > (2) if you are planning to make 'helper' capable of handling multiple > > > input files, show the error message for the ones that are not binary > > > (you would probably need a loop for that). > > > > > > I think (1) would be sufficient in this case. > > > > I too think (1) is right. It was just that originally there was $@ > > (which I now understand was wrong). So ok to apply the following patch > > on top of this series? (I assume it's ok, and will repost the whole > > thing) > > No, "helper" is supposed to be able to take multiple arguments, at least > in t4042. See the "changing textconv invalidates cache" test. The extra > argument comes from the user, not from git. Yes, I've reworked that in my v4/patch4 post. > > t/t4042-diff-textconv-caching.sh | 4 ++-- > > Why are we touching t4042 at all in this series? We are not actually > adding any tests to it, AFAICT. Because we want to catch potential wrong textconv invocation on non "bin: " files there too? Thanks, Kirill -- 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