As reported in issue2406 msg16696: @@ -66,6 +68,8 @@ self.cia = cia self.ctx = ctx self.url = self.cia.url + if self.url: + self.url += self.cia.root These two lines are wrong and should be reverted (they insert the full filesystem path inside the URL). --- This is a regression and is therefore urgent. I don't even know what a 'cia' is so the above report doesn't actually help me at all. Hopefully someone will explain what the original patch was supposed to do, what it did wrong, and why the above patch does or doesn't break its intended behavior in enough detail that someone can take some action on it. If you're on the nosy list, this means you.
For the record, "CIA" is a notification system that receives commits from open source projects and is able to dispatch them to e.g. IRC channels (we use it on #python-dev on freenode). (see http://cia.vc/)
- what the patch does wrong: as explained previously, it inserts the full local filesystem path (e.g. /data/hg/repos/cpython) inside the URL that is used in the notification messages, which is obviously wrong (the URL is supposed to point to the Web page describing the notified changeset, e.g. http://hg.python.org/cpython/rev/2ebcbdca0dee) - how the suggested change solves the issue: well, it worked for me and produced the right revision URLs - whether the suggested change is ok: not really, it breaks the test, but that's because the test has the wrong assumption IMO (the test checks that the filesystem path is indeed inside the URL). Also, the test changed from 1.8.x to 1.9 (in c322890b03e6).
I haven't actually tested the code, but it looks to me like this patch attempts to make URL construction (for creating hyperlinks to the changeset) work the same in hgcia as it does in notify, but breaks backward compatiblity with hgcia. Extensions like 'notify' have a somewhat clunky way of constructing the URL to a changeset: They expect you to provide a base URL like http://host/hg, and a 'strip' count, which prunes that number of directories from the absolute path to the repository on the filesystem before glueing the path to the URL. For example, if your repository is in /home/user/hg/repos/foo, and you serve /home/user/hg/repos as http://server/hg/, you would configure your baseurl as http://server/hg and your strip count as, I guess, 4? I didn't bother with strip in hgcia, but unfortunately I reused the baseurl setting as the entire URL displayed in the hgcia notification messages, so I suppose configuring notify and hgcia on the same repository would force notify to have a separate baseurl for every repo like hgcia does. Antoine could probably work around the change by setting cia.strip to an appropriate value. You can revert the change if you want backward compatibility for hgcia, or keep it if you want a uniform interface.
Can I get a before and after example?
btw it occurs to me there might be a hack to support both behaviours: have strip default to infinity. That way, if it isn't set, no repository path will be added to the baseurl.
This is how I imagine it's changing: repo: /home/user/hg/repos/repo repo/.hg/hgrc contains: [web] baseurl = http://server/hg/repo The URL generated in the notification message BEFORE: http://server/hg/repo/rev/hash AFTER: http://server/hg//repo/home/user/hg/repos/repo Which looks obviously bad. But the patch expects repo/.hg/hgrc like this: [web] baseurl = http://server/hg/ [cia] strip = 4 Which would produce BEFORE: http://server/hg/rev/hash AFTER: http://server/hg/repo/rev/hash Having cia.strip default to -1/infinity would produce the correct result with the original .hgrc, but the new .hgrc would also work.
Brendan's proposal (strip = -1 by default) looks fine to me.
I think we must keep the code that appends the root path to the url otherwise it is not possible to use hgcia with other extension that also use baseurl. I submitted a patch that implement the strip = -1 as suggested by pitrou like that hgcia configuration is backward compatible.
Does this go to stable? Also, the test doesn't pass. I can fix it up, and test both old and new forms.
Yep, for stable.
Here is the patch with passing test (without strip and with strip)
Fix in stable changeset: 14850:a95242af945c branch: stable user: Cédric Krier <ced@b2ck.com> date: Sat Jul 09 09:44:15 2011 +0200 summary: hgcia: Set default value of strip to -1 (issue2891)
--- Bug imported by bugzilla@serpentine.com 2012-05-12 09:21 EDT --- This bug was previously known as _bug_ 2891 at http://mercurial.selenic.com/bts/issue2891 Imported an attachment (id=1564) Imported an attachment (id=1565)