Re: [RFC/PATCH 2/2] Git-remote-mediawiki: Add push support

Jeremie Nikaes [off-list ref] writes:

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.

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$//) {
		...

+	} 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?

+			# 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";
+	}
 }