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

[Christof Petig]

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]

> 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.

> In netsync.cc:
> -  virtual ~session() {}
> +  virtual ~session() {} //  if (str_p) delete str_p; }
> ^^ eh?

looks pretty unintentional to me. Unnecessary change! <blush> Perhaps 
some intermediate debugging code left in.

> +// 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.

> +       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.

> 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.

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 ;-(

> > 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]

> 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)]

Yours
   Christof

signature.asc
Description: OpenPGP digital signature