Re: [PATCH] cvsimport: modernize and standardize external tool calling

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2010, 19:15 UTC
Message-ID: <7v8wbwzgnw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1263749749-3939-1-git-send-email-bwalton@artsci.utoronto.ca>

Ben Walton <bwalton@artsci.utoronto.ca> writes:

Show 8 quoted lines

> The cvsimport module was mixing old (git-foo) and new (git foo)
> conventions when calling git tools.  This patch standardizes the
> calling conventions used in system(), open(), exec() and backticks.
>
> Reported-by: Alexander Maier <amaier@opencsw.org>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
> This takes into account the feedback from Junio.

Hmph...

> -	open(my $fh, '|-', qw(git-update-index -z --index-info))
> -		or die "unable to open git-update-index: $!";
> +	open(my $fh, '|-', 'git update-index -z --index-info')
> +		or die "unable to open git update-index: $!";

I think this change is backwards (and there probably are others).

It used to use a shell-less one-element-per-argument list form to spawn the process that reads from the pipe, but now you are passing a single string to be split by the shell.

How about doing this as a two (perhaps three?) patch series instead?

 (1) s/git-foo/git foo/ and _nothing else_;
 (2) s/open fh, '|-', 'string'/open fh, '|-', qw(string)/ and
     s/system 'string'/system qw(string)/.