[PATCH 2 of 4] keyword: don't delete possible wrappers for commictx() (see issue2254)

Dan Villiom Podlaski Christiansen danchr at gmail.com
Mon Jul 12 08:20:26 CDT 2010


On 11 Jul 2010, at 10:10, Christian Ebert wrote:

> * Dan Villiom Podlaski Christiansen on Friday, July 09, 2010 at 10:49:27 +0200
>> Well, the issue here is that there are really two different
>> kinds of callers of commitctx(): commit() and converters.
>> commitctx() is used by various extensions — convert, hg-git and
>> hgsubversion, to name a few — to create Mercurial changesets,
>> and none of these use the commit() API. Because these
>> changesets originate outside Mercurial, it is important that
>> they be passed unmodified through commitctx() and stored as
>> such. Otherwise, the conversion has — by definition — been
>> corrupted. AFAICT all other callers of the commitctx() function
>> end up doing so through commit().
>> 
>> Granted, this matters less for the keyword extension than it
>> does for the EOL extension. Whereas the EOL extension can be
>> triggered to modify the contents of a changeset by a file in
>> the repository, the keyword extension must be explicitly
>> configured to modify files. However, it's not impossible that
>> some unlucky user of both the hgsubversion and keyword
>> extensions could run into this, if pulling from Subversion or
>> Git into a repository where they have configured keyword
>> expansion, and the contents of one of the original revisions
>> happens to trigger an expansion.
> 
> Sorry, I still don't understand.
> 
> If you "hg pull" into a hg repo with keyword enabled, nothing
> different will happen.
> 
> If you "hg pull -u" the same will happen as when you pull from a
> Mercurial repo: if the change affects a file configured for
> keyword expansion with configured keywords in it, keywords will
> be expanded in the working directory only - as they should.
> 
> Or are you saying that the EOL and keyword extensions always
> conflict - regardless of hgsubversion?

To be honest, I haven't checked that. Martin pointed out, however, some issues with how the keyword extension does its double wrapping, so I can't rule it out.

This Friday, I took a look at the source for both extensions, and I also tried unsuccessfully to see if I could get the EOL extension to affect a conversion using the convert extension. (From what I can tell, the EOL extensions is the one of the two extensions most likely to corrupt a repository.) Still, it's my experience that simply not being able to reproduce a bug doesn't mean that it doesn't exist. I can't help but worry a bit about this, though: There may not be any bugs between the interactions of the keyword extension and hgsubversion/convert, but that doesn't preclude them from cropping up in the future.

hgsubversion is a codebase that's somewhat hard to maintain. As Subversion has no notion of tags and branches — it's all just directories — hgsubversion is essentially a bunch of heuristics banged together, but with a lot of tests added so we can keep things working. When users report a bug, it can be hard enough to reproduce it even when it _doesn't_ involve other extensions. A user recently reported a case where he couldn't pull revisions that deleted files. It turned out the interactions between the EOL extension and hgsubversion were to blame.

From my perspective, there are three possible outcomes of other extensions intercepting the commitctx() calls done by hgsubversion:

1. The extension assumes this was a user-initiated commit, and does some pointless work.
2. The extension makes assumptions about the context given, that aren't valid for those created by hgsubversion.
3. The extension modifies the context, causing corruption of the conversion.

I believe the keyword extension did #1 before the double wrapper. The EOL extension is “guilty” of #2. Augie mentioned that he suspects the EOL extension of doing #3 on a user's setup.

I hope you understand why I worry about this; from our perspective, no other extensions should even _try_ to intercept our commitctx() calls. There is no advantage to doing so, and it feels like a source of bugs waiting to happen.

I have an idea for a graceful (IMHO) way of changing the wrapping which ought to fix the EOL/hgsubversion bug. I might as well apply the change to the keyword extension as well, while at it :) I'll post a patch for this, hopefully later this week.

--

Dan Villiom Podlaski Christiansen
danchr at gmail.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1943 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20100712/132c61fa/attachment-0002.bin>


More information about the Mercurial-devel mailing list