Re: [RFC/PATCH 2/2] Git-remote-mediawiki: Add push support
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 9, 2011, 17:15 UTC
- Message-ID
- <7vsjrjx6wp.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1307625360-10973-2-git-send-email-jeremie.nikaes@ensimag.imag.fr>
Jeremie Nikaes <jeremie.nikaes@ensimag.imag.fr> writes:
Show 9 quoted lines
> diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
> index 176ff09..dc1aacf 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki
> +++ b/contrib/mw-to-git/git-remote-mediawiki
> @@ -148,6 +148,14 @@ sub get_last_remote_revision {
> return $max_rev_num;
> }
>
> +sub mediawiki_filter($) {The only caller calls this function with a plain vanilla single scalar string, and this is not emulating to replace any Perl built-in. I do not see why you want to confuse the readers with a prototype here.
Show 7 quoted lines
> @@ -318,5 +327,87 @@ sub mw_import {
> }
>
> sub mw_push {
> - print STDERR "Push not yet implemented\n";
> +
> + sub push_file {The language lets you to write nested functions, but in this case I do not think it is buying you anything, other than one level unnecessarily deeper indentation to make the resulting code harder to read.
> + #$_[0] contains a string in this format : > + #100644 100644 <sha1_of_blob_before_commit> <sha1_of_blob_now> <status>\0<filename.mw>\0 > + #$_[1] contains the title of the commit message (the only phrase kept in the revision message) > + my @blob_info_split = split(/ |\t|\0/, $_[0]);
What if a filename has space or tab in it? A code that reads from "-z" output should not be using split(). Something like this (untested)?
# avoid $_[number] unless in a trivial few-liner function. they
# are unreadable.
my ($raw_diff, $message) = @_;
my ($old_mode, $new_mode, $old_sha1, $new_sha1, $status, $path) =
($raw_diff =~ /^:([0-7]+) ([0-7]+) ([0-9a-f]{40}) ([0-9a-f]{40}) (\S+)\0(.*?)\0$/) > + if (substr($complete_file_name,-3) eq ".mw"){
> + my $title = substr($complete_file_name,0,-3);
> + $title =~ s/$slash_replacement/\//g;It is probably more customary to write this like so:
if (($title = $complete_file_name) =~ s/\.mw$//) {
...Show 6 quoted lines
> + } elsif ($HEAD_sha1 ne $remoteorigin_sha1) {
> + # Get every commit in between HEAD and refs/remotes/origin/master,
> + # including HEAD and refs/remotes/origin/master
> + my $parsed_sha1 = $remoteorigin_sha1;
> + while ($parsed_sha1 ne $HEAD_sha1) {
> + my @commit_info = grep(/^$parsed_sha1/, `git rev-list --children $_[0]`);It feels extremely wasteful to traverse the whole history with rev-list every time you interate this loop. Can't you do better?
> + my $blob_infos = run_git("diff --raw --abbrev=40 -z $commit_info_split[0] $commit_info_split[1]");
> + my @blob_info_list = split(/\n/, $blob_infos);Huh? Didn't you read from "-z" output?
Show 19 quoted lines
> + # Keep the first line of the commit message as mediawiki comment for the revision
> + my $commit_msg = (split(/\n/, run_git("show --pretty=format:\"%s\" $commit_info_split[1]")))[0];
> + chomp($commit_msg);
> + foreach my $blob_info (@blob_info_list) {
> + # Push every blob
> + push_file($blob_info, $commit_msg);
> + }
> + $parsed_sha1 = $commit_info_split[1];
> + }
> +
> + print STDOUT "ok $_[1]\n";
> + print STDOUT "\n";
> +
> + # Pulling from mediawiki after pushing in order to keep things synchronized
> + exec("git pull --rebase >/dev/null");
> + } else {
> + print STDOUT "\n";
> + }
> }