Re: [PATCH 2/2] cvsimport: cleanup commit function

From: Junio C Hamano <junkio@cox.net>
Date: May 23, 2006, 08:24 UTC
Message-ID: <7vpsi55et5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <46a038f90605230113x2f6b0e4bq5a2ea97308b495e0@mail.gmail.com>

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

Show 9 quoted lines

> Jeff,
>
> good stuff -- aiming at exactly the things that had been nagging me.
> Some minor notes on top of what junio's mentioned...
>
>> +    die "unable to open $f: $!" unless $! == POSIX::ENOENT;
>> +    return undef;
>
> Heh. Is that the return of the living dead?

Note the trailing "unless" there.

Show 5 quoted lines

>> +sub update_index (\@\@) {
>> +       my $old = shift;
>> +       my $new = shift;
>
> Would it not make more sense to just pass them as plain parameters?

Meaning...? Perl5 can pass only one flat array, so the above is a standard way to pass two arrays.

Show 8 quoted lines

>> +       print "Committed patch $patchset ($branch $commit_date)\n" if
>
> Given that we have that -- should we remember it and avoid re-reading
> the headref from disk? A %seenheads cache would save us 99.9% of the
> hassle.
>
> In related news, I've dealt with file reads from the socket being
> memorybound. Should merge ok.

Merged OK, and I think your last suggestion makes sense. I'll go to bed after pushing out Jeff's two patches and yours.