From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 11 Jun 2013 15:42:05 GMT
Subject: Re: [PATCH v3 21/28] git-remote-mediawiki: Put long code into a subroutine
Message-ID: <7vli6gd582.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1370816573-3808-22-git-send-email-celestin.matte@ensimag.fr>

Célestin Matte <celestin.matte@ensimag.fr> writes:

> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   50 +++++++++++++++++----------
>  1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index c404e7b..a66cef4 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -126,28 +126,13 @@ $wiki_name =~ s{[^/]*://}{};
>  $wiki_name =~ s/^.*@//;
>  
>  # Commands parser
> -my @cmd;
> +my @cmds;

I am guessing that the new sub, parse_command, uses a local @cmd and
this is an attempt to avoid using the same name, but this renaming
of the variable is not explained.

I also wonder if you need this global @cmd/@cmds.  Instead of
passing cmdref, wouldn't it be simpler to have the helper split the
line, i.e. something like...

	sub parse_command {
		my ($line) = @_;
                my @cmd = split(/ /, $line);
		unless (defined @cmd) {
	                return 0;
		}
                ... check capabilities, list, etc....
		return 1;
	}

        while (<STDIN>) {
        	chomp;
                if (!parse_command($_)) {
			unknown command, aborting
                        last;
		}
	}