----- Forwarded message from <era@iki.fi> -----

I have been playing around with Darcs for a few months now, but my
experimentation has been hampered by a bug which manifests as "Couldn't
read patch":

 vnix$ darcs get -v mydarcs:darcs/demo
Getting the inventory...
Copying patches...
Patches copied
Repo lazily read
Repo local: "False"

Fail: Couldn't read patch Sun Nov 26 11:17:01 EET 2006  era+darcs@iki.fi
  * Initial check-in

I finally managed to track this down to different ssh versions. On the
hosts where darcs doesn't work, local administration has installed a
commercial SSH version, which doesn't support "sftp -b":

 vnix$ /usr/bin/sftp -b /tmp/darcsQ5nyCR  mydarcs:
-b requires an argument greater than zero.

 vnix$ echo $?
0

I played around with wrappers and strace and found out that the above
command line is precisely the one darcs tries to run (modulo the path to
the file with sftp commands).

At a minimim, it would be useful if the error message could be a little
more revealing. Certainly it would also be useful if Darcs could check
if the command does anything useful (like it seems to do earlier; it
runs "ssh -O an_invalid_command", apparently to see what happens) and
bail out with an error message if not.

For what it's worth, this particular sftp client (the product suite is
called "Reflection for Secure IT"; I believe it is largely compatible
with the commercial SSH2 product from SSH.com) seems to work just fine
if you use "sftp -B file" instead of "sftp -b file". Actually, you also
have to remove the colon from after the host name.

While playing around with this, I discovered that a non-zero exit code
from sftp would make darcs fail with "darcs failed:  (sftp) failed to
fetch files." so the fault really lies with the sftp implementation
which doesn't fail properly. Still, having a workaround for this would
improve the usability and credibility of Darcs.

Attached please find a script which I called sftp_darcs which I set my
DARCS_SFTP to as a workaround.

Thanks for considering this

/* era */

I'd say this is a bug, since we'd really like to be compatible with all the ssh 
implementations we possibly can.  It would be really nice, though if they were 
a bit more standard.

For what it's worth, Eric and a few others have recently been putting a lot of 
effort into getting ssh support working more portably and reporting better 
error messages.  Perhaps if we could identify this ssh, we could just use scp 
instead? It's less efficient, but if they return nonzero when there's no error, 
there's going to be no clean workaround for that.

David

On Tue, Dec 05, 2006 at 15:39:02 +0000, era wrote:

> For what it's worth, this particular sftp client (the product suite is
> called "Reflection for Secure IT"; I believe it is largely compatible
> with the commercial SSH2 product from SSH.com) seems to work just fine
> if you use "sftp -B file" instead of "sftp -b file".

Sadly, openssh's sftp treats -B as "buffer size".

> Actually, you also have to remove the colon from after the host name.

This seems reasonable.  I'll have a look.

> Attached please find a script which I called sftp_darcs which I set my
> DARCS_SFTP to as a workaround.

Could you please attach it to this ticket?

I would really like the SSH stuff to be working better.

For starters, I like your idea of transforming that control master check
(the -O an_invalid_command thing you pointed out) into a "what kind of
SSH is this?" check.  I guess the best thing would be to just run
DARCS_SSH without any options and parse the output.

Unfortunately, I'm back to working on it in my spare-spare time, i.e.,
(the spare time that's left over after my unstable duties), so this
might advance rather slowly.  If anybody wants to take over, that would
be great!

On Tue, 05 Dec 2006 21:43:26 +0000, "Eric Kow" <bugs@darcs.net> said:
> Eric Kow <eric.kow@gmail.com> added the comment:
> On Tue, Dec 05, 2006 at 15:39:02 +0000, era wrote:
> > For what it's worth, this particular sftp client (the product suite is
> > called "Reflection for Secure IT"; I believe it is largely compatible
> > with the commercial SSH2 product from SSH.com) seems to work just fine
> > if you use "sftp -B file" instead of "sftp -b file".
> 
> Sadly, openssh's sftp treats -B as "buffer size".

Then these two are precisely diametrically opposite :-/

> > Attached please find a script which I called sftp_darcs which I set my
> > DARCS_SFTP to as a workaround.
> 
> Could you please attach it to this ticket?

How? I'm trying to attach it to this reply but if that doesn't work, I'd
appreciate if somebody could point me to documentation about how to
identify myself to Roundup as "user403".

/* era */

Attachments

• sftp_darcs

On Thu, Dec 07, 2006 at 12:23:53PM +0000, era eriksson wrote:
> How? I'm trying to attach it to this reply but if that doesn't work, I'd
> appreciate if somebody could point me to documentation about how to
> identify myself to Roundup as "user403".

That worked.  You can also get login information by using the "lost your
login" link on http://bugs.darcs.net.
-- 
David Roundy
Department of Physics
Oregon State University

On Tue, 05 Dec 2006 15:47:12 +0000, "David Roundy" <bugs@darcs.net>
said:
> I'd say this is a bug, since we'd really like to be compatible with all
> the ssh implementations we possibly can.

That's laudable, but in the meantime, I think the main problem for me
was the lack of diagnostics I could understand.

  * The error message "Couldn't read patch" is not very informative, and
  googling for it only brought up a thread which said basically "it
  depends".

  * I think "--verbose" should log more, especially interactions with
  external commands, but the file system is also a good candidate.

  * For this particular problem, it would have been a nice "belt and
  suspenders" check if darcs would have checked not only the exit code
  from sftp, but also whether the files were in fact copied from the
  remote system. This would also help produce a more informative error
  message, earlier in the process.

Do you want me to submit a separate bug report for one or more of these
suggestions?

/* era */

[..]
> Do you want me to submit a separate bug report for one or more of these
> suggestions?

Unless you're using the latest release candidate (or Eric says something),
I'd take a wait and see approach, as there has recently been considerable
effort in improving the ssh infrastructure, which I think has improved
the error messages.
-- 
David Roundy
Department of Physics
Oregon State University

On Tue, Dec 05, 2006 at 21:43:26 +0000, Eric Kow wrote:
> > Actually, you also have to remove the colon from after the host name.
> 
> This seems reasonable.  I'll have a look.

One of my to-do lists told me to look into this.  Sorry for the delay,
I've finally sat down and had the requisite look.  I'm slightly nervous
about touching ssh-related code in general :-), and I notice that the
sftp code has this line:
         host = (takeWhile (/= ':') u)++":"

That is, the colon seems to be tacked on purposefully.  Now at first
glance, it doesn't really make sense that you would need that colon
there, but I'm wondering if the person that wrote the code had some
specific intention in mind.

In any case, it's code that works for the most part, and removing this
colon wouldn't fix darcs for the commercial sftp client (-b/-B) anyway.

So unless anybody wants to comment, I'm inclined to leave this one
aspect of things alone.

On Mon, 5 Mar 2007 21:26:41 +0100, "Eric Y. Kow" <eric.kow@gmail.com>
said:
> On Tue, Dec 05, 2006 at 21:43:26 +0000, Eric Kow wrote:
> > > Actually, you also have to remove the colon from after the host name.
> > 
> > This seems reasonable.  I'll have a look.
> 
> One of my to-do lists told me to look into this.  Sorry for the delay,
> I've finally sat down and had the requisite look.  I'm slightly nervous
> about touching ssh-related code in general :-), and I notice that the
> sftp code has this line:
>          host = (takeWhile (/= ':') u)++":"
>
> That is, the colon seems to be tacked on purposefully.  Now at first
> glance, it doesn't really make sense that you would need that colon
> there, but I'm wondering if the person that wrote the code had some
> specific intention in mind.
>
> In any case, it's code that works for the most part, and removing this
> colon wouldn't fix darcs for the commercial sftp client (-b/-B) anyway.
>
> So unless anybody wants to comment, I'm inclined to leave this one
> aspect of things alone.

While your hesitation is understandable, it would seem, based on a quick
Google for sftp man pages, that the colon is actually optional in all
the implementations I could quickly find.

In fact, I speculate that they allow it as a gesture towards users who
mainly use the scp(1) command, where the colon is required to indicate
which path name is remote. But it's only really required when you also
want to indicate a path on the remote host, which is hardly relevant
when receiving commands from a batch file anyway.

Here's a sampling:
  http://www.linuxcommand.org/man_pages/sftp1.html
  http://www.mkssoftware.com/docs/man1/sftp.1.asp -- "Author: Damien
  Miller", i.e. openssh
  http://www.employees.org/~satch/ssh/faq/manpages/sftp2_man.html --
  fairly ancient ssh.com version?
  http://developer.apple.com/documentation/Darwin/Reference/Manpages/man1/sftp.1.html
  -- basically identical to "linux" page
  http://amath.colorado.edu/computing/software/man/sftp.html
  http://kb.iu.edu/data/akqg.html

This one lets you browse manual pages from multiple different U*x
variants:
  http://modman.unixdev.net/index.php?manpath=OSF1-V5.1-alpha&page=sftp&sektion=1&apropos=

The Tru64 manual page has the -b/-B option inversion also found in the
version I use.

(Hmm, now will this reply go to the bug or to you, or outer space, I
wonder?)

/* era */

...
> Here's a sampling:

Looks to me like adequate research to justify the colon fix.  On the other
hand, I don't understand (and haven't read up on) the -b/-B fix, and am
unwilling to express an opinion on it (except the obvious conservative one,
that we'd by golly better not break any of our existing users).
-- 
David Roundy
Department of Physics
Oregon State University

On Wed, 07 Mar 2007 21:16:42 +0000, "David Roundy" <bugs@darcs.net>
said:
> > Here's a sampling:
> 
> Looks to me like adequate research to justify the colon fix.  On the
> other hand, I don't understand (and haven't read up on) the -b/-B fix,
> and am unwilling to express an opinion on it (except the obvious
> conservative one, that we'd by golly better not break any of our
> existing users).

As far as I know, there was no particular work on the -b/-B behavior.
There was talk of extending the "-O invalid_command" stuff to more
generally figure out which variant of SSH you're running, but I don't
believe anybody actually worked on that. (I could be wrong; I've only
been following this particular issue.)

If you have to run a DARCS_SFTP wrapper anyhow, it's way more trivial to
add a missing colon if you should ever need one, than to remove an
extraneous colon. (Not saying it can't be done, just that not everyone
is a shell wizard enough to know about ${var%:})

/* era */

I think we should be evil here and just consider this "fixed" by the new darcs
transfer-mode feature.

The user will need darcs 2.0.0 on both sides for this to work.

Marking resolved, but please complain if you disagree.