[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v6] Add a remote helper to interact with mediawiki (fetch & push)
From:       Matthieu Moy <Matthieu.Moy () grenoble-inp ! fr>
Date:       2011-08-31 17:30:25
Message-ID: vpq7h5tbia6.fsf () bauges ! imag ! fr
[Download RAW message or body]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> 2011/8/31 Matthieu Moy <Matthieu.Moy@imag.fr>:
>> 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).

>> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]