Mercurial always clobbers if repo-url contains password

[miltondp]

Buildbot 0.7.11p3 always clobber with Mercurial. When the URL of repositories are compared in function _checkRepoURL, in the file slave/commands.py, they are always different, because Mercurial obscures the password.

There is a patch for Mercurial, which adds an option for the "paths" command: --show-passwords, which doesn't change URL. It can be found here:

 http://www.selenic.com/pipermail/mercurial-devel/2009-February/010529.html

And it's necessary a small patch for Buildbot:

 http://www.miltonpividori.com.ar/files/buildbot_commands.patch

[marcusl]

better description

[dustin]

Is there a solution that doesn't require users to patch mercurial?

[marcusl]

One could always use a regex to select the part(s) of the URL that's relevant to compare. I don't have any Mercurial repo URLs with password nearby, so it's hard to say if it's feasible.

Milton: Could you give some examples?

[miltondp]

marcusl, sorry but I didn't receive any mail about your comment.

I think your idea is perfectly feasible, but maybe it would be cleaner if Mercurial had an option to show the password (it should exist, in my opinion). What you say can be implemented in the _checkRepoURL function, in the commands.py file.

Here you have an URL with a password (if that's what you meant):  http://miltondo:mypassword@myurl.com/myrepo

[marcusl]

Strange, I hope you get this though. :)

Ok, no different from ordinary HTTP URLs. (Sigh, I don't know what other type of passworded URLs I was thinking of, maybe those with green goblins in them ;)

So, pending a new feature in Mercurial (which you'd have to implement yourself, then try to get accepted, and then Buildbot still has to be backwards compatible with previous Hg versions) I'd say we move ahead with the current plan.

[marcusl]

Also, this is not really a blocker, just pretty important.

[miltondp]

Replying to marcusl:

Strange, I hope you get this though. :)

Ok, no different from ordinary HTTP URLs. (Sigh, I don't know what other type of passworded URLs I was thinking of, maybe those with green goblins in them ;)

So, pending a new feature in Mercurial (which you'd have to implement yourself, then try to get accepted, and then Buildbot still has to be backwards compatible with previous Hg versions) I'd say we move ahead with the current plan.

You are right :) I will try a solution with a regular expression to remove the user and password from the URL. I think it's not necessary for the propose of _checkRepoURL function.

[marcusl]

How about this:

buildbot/slave/commands.py

@@ -2799 +2799 @@
                 else:
                     repourl = self.repourl
 
+                def _remove_userpassword(url):
+                    if '@' not in url:
+                        return url
+
+                    from urlparse import urlparse
+                    p = urlparse(url)
+                    netloc = p[1].split('@')
+
+                    if len(netloc) > 1:
+                        return p[0] + '://' + netloc[1] + p[2]
+                    else:
+                        return p[0] + '://' + p[1]
+
+                oldurl = _remove_userpassword(oldurl)
+                repourl = _remove_userpassword(repourl)
+
                 if oldurl != repourl:
                     self.clobber = self._clobber
                     msg = "RepoURL changed from '%s' in wc to '%s' in update. Clobbering" % (oldurl, repourl)

[marcusl]

doh.. + p[2] in the else too..

[marcusl]

It's now residing here:  http://github.com/marcusl/buildbot/commits/bug640

Could you test it? It's kinda tricky writing a test-case, unless we could alter the current test to use a password-protected repo (via hg serve) by default.

[miltondp]

Great! Yesterday I wrote a solution too but I couldn't test it and I can't right now (I'll do it tomorroy). Using urlparse seems to be much better. Anyway, here it is:

--- /usr/share/pyshared/buildbot/slave/commands.py	2009-08-13 12:53:17.000000000 -0300
+++ commands.py	2009-12-27 16:49:29.783838053 -0300
@@ -2665,6 +2665,16 @@
                 else:
                     repourl = self.repourl
 
+                def _removeUserAndPasswordFromURL(url):
+                    protocol_url = url.split('://')
+                    protocol = protocol_url[0]
+                    repo_url = protocol_url[1].split('@')[-1]
+
+                    return protocol + '://' + repo_url
+
+                oldurl = _removeUserAndPasswordFromURL(oldurl)
+                repourl = _removeUserAndPasswordFromURL(repourl)
+
                 if oldurl != repourl:
                     self.clobber = self._clobber
                     msg = "RepoURL changed from '%s' in wc to '%s' in update. Clobbering" % (oldurl, repourl)

I attach a test-case also (not for _checkRepoURL, but for _removeUserAndPasswordFromURL)

[miltondp]

Test case for _removeUserAndPasswordFromURL function

[marcusl]

Be sure to test it with local paths as well. You can't assume that : is present.

[-1] is a nice trick. I'll use that in my change, and add your test. :)

[marcusl]

Here:  new bug640 branch in my repo

Your tests was a good thing, urlparse doesn't deal with ssh properly, so I had to use your code, and combine it with mine.

Are you able to test my bug640 branch properly?

[marcusl]

I rebased onto master, so use  http://github.com/marcusl/buildbot/commits/bug640 which will always be correct.

(Sorry for spamming with so many comments, I suppose google wave would rule in this context..)

[miltondp]

No problem for so many comments :)

Tomorrow I will run this code in my environment and post a comment here.

[miltondp]

It works!! Thank you very much! I tried with remote (with user and password) and local repositories. I attach both log files of the Mercurial step.

Great job, I liked a lot to help with this bug!

[miltondp]

Mercurial step - Remote repository

[miltondp]

Mercurial step - Local repository

[dustin]

Hey, cool! Marcus, I'll leave it to you to push the changes, but let me know if you'd like me to do it.

[marcusl]

commit 7e4e948a49b2f511367caac0ea91f91501fee546
Author: Marcus Lindblom <macke@yar.nu>
Date:   Tue Dec 29 11:45:02 2009 +0100

    Remove user/passwd when comparing Mercurial URLs (fixes #640)
    Thanks to Milton Pividon for doing most of the work. :)

:)

[miltondp]

Great!!!