Re: [PATCHv2] Add a remote helper to interact with mediawiki, pull & clone handled

From: Arnaud Lacurie <arnaud.lacurie@ensimag.imag.fr>
Date: Jun 7, 2011, 11:33 UTC
Message-ID: <BANLkTimTAhi6MZg_COopdyHwNifqmRVy0A@mail.gmail.com>
In-Reply-To: <BANLkTik--BAqZxCYQ-ApUn2yKmcr_-hubg@mail.gmail.com>

Hi,

2011/6/7 Thomas Adam <thomas@xteddy.org>:

> Hmm.  What guarantees can you make about the scalar value of @cmd here
> once the split has happened?  Do you not care, or do you have a
> hard-limit for split to use?

The verifications on the different cmd[x] are made in the few lines "if ... elsif... elsif ... else" right after the split.

> These explicit calls to "STDOUT" here -- why?  It's redundant typing
> and unsightly, and certainly not very perl-y.  Is there some reason
> why you're using this style?

We wanted to highlight the fact that we are sending those lines to STDOUT which is piped into the caller of the remote helper.

Show 8 quoted lines

>
>> +       print STDOUT "? refs/heads/master\n";
>> +       print STDOUT '@'."refs/heads/master HEAD\n";
>> +       print STDOUT "\n";
>
> Hmm.  Why not just:
>
> print STDOUT '@'."refs/heads/master HEAD\n\n";

Here, we wanted to stick to the convention given by the remote-helper documentation that indicates the list command should end with a blank line.

Show 5 quoted lines

>
> There's certainly a large number of perl-specific clean-ups I'd be
> inclined to do -- and if I get time later, I might show you how.  But
> don't let this necessarily put off this patch for inclusion or
> anything like that.

As said in previous mails, we are relatively new using perl (it's been roughly two weeks) so any advice on how to get things more perl-ish are welcome.

Thanks for your feedback

-- 
Arnaud Lacurie