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

Dan Villiom Podlaski Christiansen danchr at gmail.com
Fri Jul 9 03:49:27 CDT 2010


On 8 Jul 2010, at 00:18, Christian Ebert wrote:

> As this is still somehow way over my head, may I just say that
> from a "pure keyword extension perspective" the following passes
> test-keyword:
> 
> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -485,16 +485,7 @@
>             data = super(kwrepo, self).wread(filename)
>             return kwt.wread(filename, data)
> 
> -        def commit(self, *args, **opts):
> -            # use custom commitctx for user commands
> -            # other extensions can still wrap repo.commitctx directly
> -            self.commitctx = self.kwcommitctx
> -            try:
> -                return super(kwrepo, self).commit(*args, **opts)
> -            finally:
> -                del self.commitctx
> -
> -        def kwcommitctx(self, ctx, error=False):
> +        def commitctx(self, ctx, error=False):
>             n = super(kwrepo, self).commitctx(ctx, error)
>             # no lock needed, only called from repo.commit() which already locks
>             if not kwt.record:
> 
> 
> Of course this looks much simpler to my simpleton eyes because
> I don't know where else besides from within commit commitctx is
> needed. Sorry if I overlooked something obvious.

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. For the EOL extension, the scenario is simpler: having a .hgeol file in a Subversion or Git repository means that the EOL extension is likely to corrupt the conversion.

An alternate, and perhaps slightly less fragile solution to this problem might be to introduce a ‘usercommitctx’ method to localrepo. The standard implementation would do nothing but call commitctx() with the exact same arguments. We could then modify commit() to call it. This would make the distinction between ‘user commits a context’ and ‘commit a raw context’ explicit, and grant EOL and keyword a simple means of only intercepting the former.

--

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/20100709/44ebc751/attachment.bin>


More information about the Mercurial-devel mailing list