Thanks for working on this, this approach if far better and far more mergeable than your previous one :)
The redirection handling in itself was designed as is because the idea is that the redirections must be followed only for the first query ever made for a branch. Either (as you found out) for a POST(.bzr/smart) or (as you can observe by prepending nosmart+ to your url) for GET(.bzr/branch-format) or GET(./bzr/checkout/format).
I think your diagnosis is sound about updating the transport only if no errors finally occurs. On the other hand, errors may also occur later so your approach may have to be deployed in other places. But I think your patch may not need to address these concerns now.
Your patch needs to be reviewed before inclusion though, so please send it to <email address hidden> (you may need to subscribe).
Before doing so though, a couple of remarks:
- you changed the bzr behavior, so you need tests to avoid future regressions,
- you fixed a bug, so we like at least one test failing before your modifications and succeeding after your modifications,
- you added a public method (find_transport_and_format), there is an API stability policy regarding public methods, it may be more appropriate to use a private method here (otherwise this method needs more tests),
- be aware that some plugins use or redefine this method (foreign VCS plugins as bzr-svn, bzr-hg, bzr-git, etc) so it will be nicer for them if you could find a way to stay compatible,
- it seems a bit strange that you need to modify workingtree at all, but other reviewers may know better than me.
Regarding the tests, have a look in bzrlib/tests at http_server.py and test_http.py on how to implement an http server with unusual behavior (like users.skynet.be).
Thanks for working on this, this approach if far better and far more mergeable than your previous one :)
The redirection handling in itself was designed as is because the idea is that the redirections must be followed only for the first query ever made for a branch. Either (as you found out) for a POST(.bzr/smart) or (as you can observe by prepending nosmart+ to your url) for GET(.bzr/ branch- format) or GET(./bzr/ checkout/ format) .
I think your diagnosis is sound about updating the transport only if no errors finally occurs. On the other hand, errors may also occur later so your approach may have to be deployed in other places. But I think your patch may not need to address these concerns now.
Your patch needs to be reviewed before inclusion though, so please send it to <email address hidden> (you may need to subscribe).
Before doing so though, a couple of remarks:
- you changed the bzr behavior, so you need tests to avoid future regressions, _and_format) , there is an API stability policy regarding public methods, it may be more appropriate to use a private method here (otherwise this method needs more tests),
- you fixed a bug, so we like at least one test failing before your modifications and succeeding after your modifications,
- you added a public method (find_transport
- be aware that some plugins use or redefine this method (foreign VCS plugins as bzr-svn, bzr-hg, bzr-git, etc) so it will be nicer for them if you could find a way to stay compatible,
- it seems a bit strange that you need to modify workingtree at all, but other reviewers may know better than me.
Regarding the tests, have a look in bzrlib/tests at http_server.py and test_http.py on how to implement an http server with unusual behavior (like users.skynet.be).