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

[Christian Ebert]

* Dan Villiom Podlaski Christiansen on Monday, July 12, 2010 at 15:20:26 +0200
> On 11 Jul 2010, at 10:10, Christian Ebert wrote:
>> 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.

I can see this being a danger.

> 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;

Of course.

> 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'm not up to the major act to install svn python bindings, so I
can't try out hgsubversion.

Here are just a few more simpleton observations:

For the convert extension commitctx is called only twice, and
only from within convert/hg.py, in the context of tags and an
"octopus" merge if I am not mistaken.

So far, whenever I used hg convert, the working directory was
never touched or updated. Are there use cases when this happens?
If not, the call to keyword's commitctx would be noticed by a
change in the working directory. For some time, albeit a while
ago, I converted the vim cvs repo regularly with keyword
expansion enable for all files. There was never any write action
to the working directory.

Note that my failure to exactly grasp the problem shouldn't
prevent you in any way from going forward with your patch.
Ideally I'd like to create a test case where there is breakage
without the (double)wrapper. I'd already like that for the
current state (kwcommitctx instead of simply overriding
commitctx).

c
-- 
theatre - books - texts - movies
Black Trash Productions at home: http://www.blacktrash.org/
Black Trash Productions on Facebook:
http://www.facebook.com/blacktrashproductions