Merge lp://staging/~garyvdm/bzr-upload/nowt into lp://staging/bzr-upload

There is a TODO about handling chmod bits (waiting for more support from bzr itself).

The idea is to take the chmod bits in the local working tree to replicate them in the remote working tree.

Not having a working tree will therefore make that impossible.

I'm curious though to understand your use case here, can you explain it ?

On Thu, Sep 18, 2008 at 8:07 AM, vila
<<email address hidden><v.ladeuil%<email address hidden>>
> wrote:

> Vote: Disapprove
> There is a TODO about handling chmod bits (waiting for more support from
> bzr itself).
>
> The idea is to take the chmod bits in the local working tree to replicate
> them in the remote working tree.
>
> Not having a working tree will therefore make that impossible.
>
> I'm curious though to understand your use case here, can you explain it ?
> --
> https://code.edge.launchpad.net/~garyvdm/bzr-upload/nowt/+merge/1071<https://code.edge.launchpad.net/%7Egaryvdm/bzr-upload/nowt/+merge/1071>
> You are subscribed to branch ~garyvdm/bzr-upload/nowt.
>

Use Case:
We have more than one person working on a website. The "central" copy of the
branch is stored on the website. I want it so that when we commit locally,
it does not upload. But when you push to the "central" branch, it
automatically uploads.

So with the above patch, I was able to run
bzr upload ftp://mysite/ --directory=ftp://mysite/ --auto

And now it uploads when you push to ftp://mysite/, but not when you commit
locally.

Gary van der Merwe:
>So with the above patch, I was able to run
>bzr upload ftp://mysite/ --directory=ftp://mysite/ --auto

Wow.

I'm always impressed when people use tools in ways that weren't anticipated :D

Everywhere in the plugin we talk about a *local* branch and a remote site, you twist that by using a *remote* branch and a remote site...

That's indeed very interesting and a valuable workflow.

Your patch is simple and clearly demonstrate that the code is ready to handle that workflow, I'm now convinced that we should merge your proposal.

But I'd really like updates on the documentation (as small as it is right now :) and more importantly on the tests before we merge it.

Would you like to continue working in that direction ?

Regarding my objection about chmod bits requiring a workiing tree, I think the objection remains.
But since chmod bits handling requires client and server cooperation, it means your workflow has ways to address the problem.

So :
1) can you tell us how your address the chmod bits problem ?
2) once we implement that, we'll have to check for a working tree availability and fallback gracefully when none is available

Gary ? Are you still interested in working on that ?

I meant, Gary, your fans are waiting ! Look: https://bugs.launchpad.net/bzr-upload/+bug/281256

Yhea - I am still interested - but I'm extremely busy at work. I don't
know when I will be able to work on it.

On Fri, Oct 10, 2008 at 3:30 PM, vila <email address hidden> wrote:
> I meant, Gary, your fans are waiting ! Look: https://bugs.launchpad.net/bzr-upload/+bug/281256
> --
> https://code.edge.launchpad.net/~garyvdm/bzr-upload/nowt/+merge/1071
> You are subscribed to branch ~garyvdm/bzr-upload/nowt.
>

Hi

I've updated the documentation, and added tests for testing uploading from a remote branch with no working tree.

With regards to chmod: I feel that if you have data, or metadata that you would want to apply it somewhere else, that should probably be stored and versioned in the repository. How ever, if this does not happen for what ever reason, it would be very easy to make code that looks for chmod bit in the working tree to no run if there is no working tree.

Please review.

Regards,

Gary

Vila, this one is all yours ;)

You added a lot of extra spaces at end of lines and even spaces on otherwise empty lines, don't do that please.

=== modified file 'README'
--- README 2008-08-01 18:59:59 +0000
+++ README 2009-01-07 10:33:57 +0000
@@ -57,6 +57,12 @@
 will enable the hook for this branch. If you were to do a commit in this branch
 now you would see it trigger the upload automatically.

+If you keep push branch to the remote location that you upload to, and you only
+want to upload when the tip of the remote branch changes, you can set the hook
+

Do you mean 'keep pushing your branch' ?

Anyway, I'm a bit worried that this example will confuse users more than showing the use case: you're in fact using upload to update the working tree of the remote branch. Which may give the wrong impression that we know how to do that in the general case (which isn't true for symlinks for example).

=== modified file 'tests/test_upload.py'
--- tests/test_upload.py 2008-09-16 06:51:08 +0000
+++ tests/test_upload.py 2009-01-07 13:58:22 +0000
@@ -18,7 +18,7 @@
      workingtree,
+ trace,
     )

You don't use trace AFAICS, so don't import it.
 from bzrlib.smart import server as smart_server
+from bzrlib.push import _show_push_branch

Urgh, using private functions from bzrlib is definitely not acceptable. Looking closer to your branch history, it seems that you encounter problems due to:

  up_url = self.get_transport(self.upload_dir).external_url()

use:

  up_url = self.get_url(self.upload_dir)

instead so you'll get the proper url.

+ try:
+ self.assertFalse(self.get_upload_auto())
+ except errors.NotBranchError:
+ pass

Erm, that's not the way you're supposed to make tests succeed :-)

+
+ def do_full_upload(self, *args, **kwargs):
+ up_url = self.get_transport(self.upload_dir).external_url()
+
+ rev_id = self.branch.last_revision()
+ output = StringIO()
+ _show_push_branch(self.branch, rev_id, up_url, output)
+

Apart from using a private function, you're pushing to the upload_dir whose contents is the same as a working tree, using a different directory for that purpose will make things clearer.

Finally regarding the auto tests I suspect you should read them carefully to ensure *when* the hooks are triggered or you will no test what you think you test :)

On Wed, Jan 7, 2009 at 4:36 PM, vila <email address hidden> wrote:
> You added a lot of extra spaces at end of lines and even spaces on otherwise empty lines, don't do that please.

Fixed. The spaces on empty lines were put there by my IDE (Komodo.)

> === modified file 'README'
> --- README 2008-08-01 18:59:59 +0000
> +++ README 2009-01-07 10:33:57 +0000
> @@ -57,6 +57,12 @@
> will enable the hook for this branch. If you were to do a commit in this branch
> now you would see it trigger the upload automatically.
>
> +If you keep push branch to the remote location that you upload to, and you only
> +want to upload when the tip of the remote branch changes, you can set the hook
> +
>
> Do you mean 'keep pushing your branch' ?

I had to read that a few time time to understand what I meant! I've
made it clearer now.

> Anyway, I'm a bit worried that this example will confuse users more than showing the use case:

I've move this to a new section to try make it more clear.

>you're in fact using upload to update the working tree of the remote branch. Which may give the wrong impression that we know how to do that in the general case (which isn't true for symlinks for example).

Updating the working tree is not my use case. You are updating a tree,
and like a working tree, that tree sits in the same dir as its branch.
But it is not a working tree, so you can't do operations on it that
require a working tree, such as commit or merge.

However, you made me realize that if the user were to try upload to a
location where there is a working tree, it would cause big problems.
So I have added a check, and a corresponding test, to make sure that
the dir that you are uploading to is not a working tree.

>
> === modified file 'tests/test_upload.py'
> --- tests/test_upload.py 2008-09-16 06:51:08 +0000
> +++ tests/test_upload.py 2009-01-07 13:58:22 +0000
> @@ -18,7 +18,7 @@
> workingtree,
> + trace,
> )
>
> You don't use trace AFAICS, so don't import it.

Fixed.

> from bzrlib.smart import server as smart_server
> +from bzrlib.push import _show_push_branch
>
> Urgh, using private functions from bzrlib is definitely not acceptable.

I have gone back to using cmd_push.run. The problem with this is that
it print "Pushed up to revision X." messages while you are running the
test, and I was not able to find an alternative way to using the
_show_push_branch method to make it not do this.

> Looking closer to your branch history, it seems that you encounter problems due to:
>
> up_url = self.get_transport(self.upload_dir).external_url()
>
> use:
>
> up_url = self.get_url(self.upload_dir)
>
> instead so you'll get the proper url.

That wasn't having a problem there, but that is simpler, so I'll us
it. How come we don't use that elsewhere in test_upload.py?

>
> + try:
> + self.assertFalse(self.get_upload_auto())
> + except errors.NotBranchError:
> + pass
>
> Erm, that's not the way you're supposed to make tests succeed :-)
...
> Finally regarding the auto tests I suspect you should read them carefully to ensure *when* the hooks are triggered or you will no test what you think you test :)

Fix...

Sorry for the delay, the week was quite busy for me.

>>>>> "Gary" == Gary van der Merwe writes:

    Gary> On Wed, Jan 7, 2009 at 4:36 PM, vila <email address hidden> wrote:
    >> You added a lot of extra spaces at end of lines and even spaces on otherwise empty lines, don't do that please.

    Gary> Fixed.

Not yet, there are still some, as well as line longer than 80 in
test_upload.py.

    Gary> The spaces on empty lines were put there by my IDE
    Gary> (Komodo.)

I don't know Komodo, but I think a friend of mine told me about
some way to put a vertical line at the 80 limit at least, no idea
for the spaces-only lines though.

    Gary> I had to read that a few time time to understand what I
    Gary> meant! I've made it clearer now.

Indeed the wording is clearer.

    >> Anyway, I'm a bit worried that this example will confuse
    >> users more than showing the use case:

    Gary> I've move this to a new section to try make it more clear.

That's good.

    >> you're in fact using upload to update the working tree of
    >> the remote branch. Which may give the wrong impression
    >> that we know how to do that in the general case (which
    >> isn't true for symlinks for example).

    Gary> Updating the working tree is not my use case. You are
    Gary> updating a tree, and like a working tree, that tree
    Gary> sits in the same dir as its branch. But it is not a
    Gary> working tree, so you can't do operations on it that
    Gary> require a working tree, such as commit or merge.

    Gary> However, you made me realize that if the user were to
    Gary> try upload to a location where there is a working tree,
    Gary> it would cause big problems. So I have added a check,
    Gary> and a corresponding test, to make sure that the dir
    Gary> that you are uploading to is not a working tree.

That's good too.

But for the sake of clarity, please use two different directories
in your example or I can guarantee you that users will still be
confused and think we really update their working tree, no matter
how many times you can repeat the contrary.

Besides, since you've proving that a remote branch can be used to
upload a remote tree (both being totally separated), we'd better
just show it in the example or people may well think that it
works only if the branch and the remote tree are at the same
location.

    >>
    >> === modified file 'tests/test_upload.py'
    >> --- tests/test_upload.py 2008-09-16 06:51:08 +0000
    >> +++ tests/test_upload.py 2009-01-07 13:58:22 +0000
    >> @@ -18,7 +18,7 @@
    >> workingtree,
    >> + trace,
    >> )
    >>
    >> You don't use trace AFAICS, so don't import it.

    Gary> Fixed.

Good.

    >> from bzrlib.smart import server as smart_server
    >> +from bzrlib.push import _show_push_branch
    >>
    >> Urgh, using private functions from bzrlib is definitely not acceptable.

    Gary> I have gone back to using cmd_push.run. The problem
    Gary> with this is that it print "Pushed up to revision X."
    Gary> messages while you are running the test,

Ugly :-/

    Gary> and I was not able to find an alternative way to using
    Gary> the _show_push_branch method to...

On Sun, Jan 18, 2009 at 9:48 PM, Vincent Ladeuil <email address hidden> wrote:

> Sorry for the delay, the week was quite busy for me.

I know the feeling..

> Since we don't care about what happen *inside* that push and we
> are pretty sure it will never fail, the remark in
> test.upload.TestUploadMixin._get_cmd_upload doesn't apply (we
> won't have to debug code with redirected output), you can just
> use run_bzr like you did in your first attempt !

Hmm - thats what I tried originally, but when I was running the test,
it was "hanging". After some debugging, I discovered that it was
asking for a password. Looking back now, I see that I had a syntax
error in the args to run_bzr that was causing that.

> Which leads me to this: reusing the existing tests isn't the
> right approach, you don't need to re-test all the upload logic
> just because we now use a remote branch, only a few dedicated
> tests are needed. I'm sorry I didn't notice that sooner :-/

Sorry about this. This was the first time ever that I have written unit tests.

> Please look at
> lp:~bzr-upload-devs/bzr-upload/upload-from-remote-branch and tell
> me what you think.
>
> If you're ok, I'll merge that, if not, join bzr-upload-devs and
> work from there :)

All of the code look good to me.

+Note that you will consume more bandwith this way than uploading from
+private.example.com.

I found this a bit confusing. Can me maybe say:

+Note that you will consume more bandwith this way than uploading from a local
+branch.

I would like to point out the value of this by saying:

+This, together with --auto, can be use to enable a hook that uploads when you
+push to your central branch, rather than when you commit to your local branch.

I also remove the known issue regarding x bits as it has been fixed.

I've applied to join bzr-upload-devs. In the mean time, I've attached
a bundle with my suggested changes.

Regards,

Gary