Re: [PATCH v6] Add a remote helper to interact with mediawiki (fetch & push)

Sverre Rabbelier [off-list ref] writes:

Heya,

2011/8/31 Matthieu Moy [off-list ref]:

quoted

So, after understanding better how import works, here's an updated
patch that gets rid of the hacky workaround to terminate and send the
"done" command at the right time.

So what do you think of the way the protocol works now? Do you agree
that (modulo lacking docs) it is better than previously?

I'm not sure I understood exactly how it was before, but the current
protocol seems indeed at least reasonable:

* It's possible to specify a batch of imports, so the remote-helper has
  freedom to optimize the import of multiple refs.

* A batch of import is clearly delimited, both on stdin and stdout, so
  it is possible to alternate import batches and other commands.

I still have a few complaints, because even with a better doc, I still
found the debugging a bit hard. To make it easy for remote-helpers
authors, I think the transport-helper could have an explicit "done"
command, so that the command stream look like

import foo
import bar
\n
done

instead of

import foo
import bar
\n
\n

and to let the remote-helper's code be like

while($cmd = <read command>) {
   if ($cmd eq "command1") {
       do something;
   } elsif ($cmd eq "command2") {
       something else;
   } elsif ($cmd eq "done") {
       exit properly;
   }
}

I'm not sure whether changing this now is worth the trouble though.

I'd have appreciated if the transport-helper had given me an explicit
error message when writting to a broken pipe too. I finally got it with
gdb, but lost some time trying to understand (especially painfull since
there was a race condition between the remote-helper termination and git
writting to it, so the bug wasn't reproducible).

quoted

Actually, push had the same problem but it just went unnoticed (the
remote has just one branch, so it's silly to try to push multiple
branches at the same time ...). This version handles push more
cleanly, giving accurate error message in cases like

 git push origin :master
 git push origin foo bar master

or perhaps more commonly

 git push --all

in a repository with branches other than master.

My perl skills are minimal, but I'm curious how/where you implemented
this?

Here:

+	for my $refspec (@refsspecs) {
+		unless ($refspec =~ m/^(\+?)([^:]*):([^:]*)$/) {
+			die("Invalid refspec for push. Expected <src>:<dst> or +<src>:<dst>");
+		}
+		my ($force, $local, $remote) = ($1 eq "+", $2, $3);

At this point, $force is a boolean saying whether there were a +, and
$local and $remote are as you can guess.

+		if ($force) {
+			print STDERR "Warning: forced push not allowed on a MediaWiki.\n";
+		}
+		if ($local eq "") {
+			print STDERR "Cannot delete remote branch on a MediaWiki\n";
+			print STDOUT "error $remote cannot delete\n";

print STDERR goes to the console (i.e. to the user), and print STDOUT
goes to the Git's transport-helper.

+			next;
+		}
+		if ($remote ne "refs/heads/master") {
+			print STDERR "Only push to the branch 'master' is supported on a MediaWiki\n";
+			print STDOUT "error $remote only master allowed\n";
+			next;
+		}

Is this something that we can port to remote-testgit to document the
CPB on handling such things?

CPB = ?

Actually, my case is very particular, since the only thing to do with
branches is to make sure the user doesn't use them. In remote-testgit,
there are actually branches.

And testgit use the undocumented "export" feature, which does not seem
to support branch deletion:

git push origin :branch2
fatal: remote-helpers do not support ref deletion
moy@bauges:/tmp/clone$ Traceback (most recent call last):
  File "/home/moy/local/usr-squeeze/libexec/git-core/git-remote-testgit", line 252, in <module>
    sys.exit(main(sys.argv))
  File "/home/moy/local/usr-squeeze/libexec/git-core/git-remote-testgit", line 249, in main
    more = read_one_line(repo)
  File "/home/moy/local/usr-squeeze/libexec/git-core/git-remote-testgit", line 215, in read_one_line
    sys.stdout.flush()
IOError: [Errno 32] Broken pipe

(This comes from

transport-helper.c:750:                 die("remote-helpers do not support ref deletion");

called before starting the exporter)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/