Re: [Monotone-devel] new ssh branch, success!

[Nathaniel Smith]

On Sat, Nov 20, 2004 at 04:51:28PM +0100, Christof Petig wrote:
> Nathaniel Smith schrieb:
> >On Wed, Nov 17, 2004 at 06:06:03PM +0100, Christof Petig wrote:
> >
> >>I created a new net.venge.monotone.ssh checkin which merges the
> >>changesets branch and the ssh branch and fixes the remaining issues.
> >>
> >>See yourself:
> >
> >[...]
> >
> >Cool!
> >
> >Why did you do it by creating an independent fork of .changesets and
> >leaving .ssh with two unrelated heads?  Is there some reason that
> >  $ monotone propagate net.venge.monotone.changesets net.venge.monotone.ssh
> >didn't/wouldn't work, or?
> 
> nv.monotone.changesets had two heads at the time in my database, so 
> propagate could not work (IIRC). Also nvm.cs forked earlier than nvm.ssh 
> from nv.monotone (and I did not want to propagate unrelated changes in 
> nv.m to nvm.cs. [Excuse my abbreviations, I hope they are unambiguous]

Ah, makes sense.  The former is sorta a small UI bug, the latter is a
side-effect of the weird "switching mainlines" thing that we seem to
be doing, and not a general problem... cool.

> >Hmm, might be some tweaking possible with these, but more important to
> >get something working and in use.
> 
> Actually I think that using ssh port forwarding is superior to starting 
> a server on the fly (e.g. concurrency, starting overhead). But having 
> the option to use monotone sync without a running server is a win.

Agreed on all counts, I think.  (You also may get better security; one
of the big problems with CVS is that if I have write access to a
repository, I really have arbitrary write access to the repository, so
I can do things like go and remove old changes, etc., and there's no
way to audit that... with ssh forwarding and monotone, you need to
have some shell access to the server, but you're actually talking to a
netsync server run under a different uid, and that's the uid that has
arbitrary write access to the database.  ...Of course, one could do
something similar by enhancing the start-on-the-fly version to run a
suid script or something.)

> >
> >+// TODO: we should check for errors
> >^^ eh?
> 
> fork might return -1. (process limits etc) Printing errno instead of 
> failing with a broken pipe would be nicer but does not make much difference.

I guess what I meant was, "can we fix this?" :-)

> >
> >+       execvp(newargv[0],(char*const*)newargv);
> >^^ I don't know if we have a standard convention on this, but _I_ like
> >new templatey cast operators.  (BTW, what's this cast doing anyway?)
> 
> converting from const char ** to char * [const] * is not possible using 
> const_cast IIRC. And &const_cast<char*>(*newargv) is not better either 
> IMHO. IMHO this is a bug in the specification of execvp (or in my 
> understanding of const_cast).
> 
> I prefer new style casts, too.

Hmm, no, checking the standard, you can const_cast from "const char**"
to "char *const *".  (You can also just const_cast to "char **", and
implicit conversion will take you from "char **" to "char *const *".

> >Generally, your added code has somewhat odd formatting?  In things like:
> >
> >+      while (sess->arm())
> >+      {  if (!sess->process())
> >
> >do we really need the 'if' tucked up on the same line as the '{'?
> 
> Of course we dont need that. Never argue about taste (or indentation 
> style). If needed I will reindent it according to monotones standards.
> 
> >And overall, if you could put spaces in appropriate places, that would
> >be nice; it feel pretty crammed to see things like:
> >
> >+     newargv[newargc++]="-";
> >+     for (unsigned i=0; i<newsize-newargc-2 && i<collections.size(); ++i)
> >+       newargv[newargc++]=collections[i]().c_str();
> >+       newargv[newargc]=0;
> 
> With my editor it looks like
>         for (unsigned i=0; i<newsize-newargc-2 && i<collections.size(); ++i)
>           newargv[newargc++]=collections[i]().c_str();
>         newargv[newargc]=0;
> which is ok IMHO.

Oops, mine too; messed up the formatting transferring to email.

I'm not terribly picky about style, and I'm not sure that Monotone has
a uniform style, but both of these things make the code really hard to
read for me -- I've never encountered FOSS code using that brace
convention, and the lack of spaces just makes my eyes bleed.  So I at
least would prefer if you'd fix these things :-)

i.e., something like:
    for (unsigned i = 0; i < newsize - newargc - 2 && i < collections.size(); 
++i)
      newargv[newargc++] = collections[i]().c_str();
    newargv[newargc] = 0;
would be a vast improvement.

> Running astyle over the code is always a good idea for projects 
> contributed by several people. But to do that you have to agree on the 
> correct settings ;-(

And have to do it consistently, or you get massive merge conflicts
every time you do...

> >>If you specify --debug the stderr from the server call (also with 
> >>--debug) will go into monotone-server.log .
> >
> >
> >What does this mean, exactly?  Where does this file end up?  On the
> >server or the client?  It seems like it would be more polite to have
> >the local monotone report the errors in some clearly marked way,
> >rather than randomly littering files in the cwd?  And usually we want
> >to see error messages even if we aren't using --debug, --debug is very
> >very chatty.
> 
> It will end up on the client. Debugging two stderrs from two monotone 
> processes on different machines intermingled in one file is no fun. This 
> way debugging the server was easiest to me. I would never advertize this 
> as _the_ way to do it.
> 
> You will see error messages if you omit debug (unless they are sent to 
> stdout which will get interpreted as an illegal netsync packet).
> 
> >How about if the remote server is always run with --quiet, and the
> >local monotone always prints any remaining output like
> >  monotone: remote server said: ...
> >or somesuch?
> 
> This means to intercept the stderr to print it. Not overly compelling to 
> me. [you need to select+read on the filedescriptor in addtion to 
> monotone's normal operation that's not minimal-invasive]

Yes.  IMHO, sometimes that's the price of correctness :-) The
monotone-error.log thing is kind of a hack, and people will
_definitely_ want to see these errors, debugging without them is
frustrating...

> >What happens with this patch on Windows?  Will it compile?  Will it
> >run?  (Maybe someone else with a Windows dev environment and/or a
> >stake in the outcome should look into this?)
> 
> Sadly it will not compile. Windows does not provide fork, execvp and 
> pipe. Since things are totally different there (CreateProcess, 
> CreatePipe), I did not address this in my first attempt [To be honest I 
> do not look forward to writing this code, though I'm able to do it (I 
> never used CreatePipe so far)]

Ah, so you're planning to do this?  Excellent!

IMHO the requirements for integrating this patch in mainline should be
that it works (or at least doesn't break the build) on Windows, and
has readable code... that's enough to go on.

-- Nathaniel

-- 
"If you can explain how you do something, then you're very very bad at it."
  -- John Hopfield