[MERGE] Transport.non_atomic_put

[Martin Pool]

On 18 Aug 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:

> >> +    def test_non_atomic_put(self):
> >> +        t = self.get_transport()
> >> +
> >> +        if t.is_readonly():
> >> +            self.assertRaises(TransportNotPossible,
> >> +                    t.non_atomic_put, 'a', StringIO('some text for a\n'))
> >> +            return
> > 
> > So perhaps this should be split out into a separate
> > test_nonatomic_put_on_readonly_fails, since it is a separate
> > requirement.  Maybe that's taking it too far.
> 
> Well, these tests are run across all permutations. We need to check
> is_readonly() and return. I can have it be a separate test, which just
> returns for non-readonly transports.
> 
> This was how other functions were tested. But I can do it either way.

If it's consistent with the others it's OK this way.

> > Will the caller usually have a file-like object already?  Perhaps we
> > should have non_atomic_put_bytes, so as to avoid creating and unwrapping
> > a StringIO object.  Perhaps that should be deferred and done to all
> > methods at once.
> 
> Not sure. So far, the callers have had strings, and lines of strings. So
> we would still need to collapse the lines of strings.
> 
> But yes, all callers so far have something in memory, not a real
> file-like object.
> 
> ...

OK, so perhaps we should separately consider put_bytes,
non_atomic_put_bytes, etc?

> > What do you mean by 'sticky bit' here?  To me it is the 01000 bit, and
> > there's no such thing as the 'group sticky bit', and anyhow the umask
> > never affects them.  I don't see why we'd be wanting to set either the
> > sticky bit or the setgid bit on plain files...
> 
> Right. I was commenting the wrong place. The real problem with the
> sticky bit is on directories, not on files. That is the 02000 bit.

That's called the setgid but, not the sticky bit, fyi.

> So we *might* want to remove a lot of the permission code, and just fall
> back to expecting people setting umask correctly. I don't really think
> we want to remove permissions, because it still helps for people with
> local shared directories. But it doesn't help as much as I would like.
> 
> ...
> 
> > Telling users to set the umask properly might be OK locally but it may
> > not be easy for an sftp server - they'll need to work out what shell
> > script, if any, gets run before the sftp server runs.  I think in many
> > cases they would need the sftp server's sysadmin to change it globally.
> 
> Right. But we are screwed either way.
> https://launchpad.net/products/bzr/+bug/50568

Ah, I see what you mean.  I think we are.

Perhaps this should be considered a bug in openssh's sftpd though.
Allowing g+s on directories seems fairly safe.

> > Should the close() be in a finally block?
> > 
> 
> I'll look close at the close(). But this is in SFTP, if we have an
> exception, we may have already lost the connection. But the old _put()
> code uses an exception catcher to ensure the file is closed, so I can
> make sure to close in a finally here.

You're probably right - it's just a standard thing I check in reviews.

-- 
Martin