[PATCH 2 of 2 RFC] mq: ignore subrepos (issue2499)

[Kevin Bullock]

On 16 Nov 2010, at 11:58 PM, Nicolas Dumazet wrote:

> On Tue, 16 Nov 2010 13:13:31 -0600
> Kevin Bullock <kbullock+mercurial at ringworld.org> wrote:
> 
>> # HG changeset patch
>> # User Kevin Bullock <kbullock at ringworld.org>
>> # Date 1289934367 21600
>> # Node ID 98ef79b835aab117950f320da3abb06c2d013a58
>> # Parent  742ab7cea58e8ca3aaea5d5d4b4fa0e252f9389e
>> mq: ignore subrepos (issue2499)
>> 
>> If MQ allows modifying .hgsub or .hgsubstate in a patch, it can easily
>> lead to an inconsistent subrepo state. This patch prevents qrefresh from
>> adding any modifications to .hgsub or .hgsubstate to a patch. The user
>> is warned that these files are not included in the patch.
> 
> Good. Thanks for working on this, I like the idea.

Thanks for looking over the patch!

>> The tests test both the slightly irrational and the pathological cases.
>> 
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -1298,12 +1298,18 @@
>>             # local dirstate. in this case, we want them to only
>>             # show up in the added section
>>             for x in m:
>> +                if x == '.hgsub' or x == '.hgsubstate':
> 
> But I would rather see everywhere "x in ('.hgsub', '.hgsubstate')".

That would do as well and looks cleaner. Python is not my native language :)

> Or do we want to use the exclude parameter of match objects? we always give
> such objects to status() calls, we could use them to exclude .hgsub and 
> .hgsubstate instead of matching manually filenames?

I had thought of that, but then how do we detect that .hgsub{,state} is matched to print a warning?

> By the way, would we like to see a "mqignore" kind of feature, or at least,
> a module-wide variable? say, a list of patterns that can be used as exclude
> parameters to match objects?
> That would allow extensions to extend the list, to add files to be ignored?

I thought of doing something like that too—adding a general facility to list files that should never be included in patch queues—but decided to try the simplistic implementation first.

>> diff --git a/tests/test-mq-qrefresh.t b/tests/test-mq-qrefresh.t
>> --- a/tests/test-mq-qrefresh.t
>> +++ b/tests/test-mq-qrefresh.t
>> @@ -487,3 +487,76 @@
> 
> [...]
> 
>> 
>> +test when deleting
>> +  $ rm .hgsub .hgsubstate
>> +  $ hg qrefresh
>> +  warning: not removing .hgsub
>> +  warning: not removing .hgsubstate
>> +  refresh interrupted while patch was popped! (revert --all, qpush to recover)
>> +  abort: No such file or directory: $TESTTMP/repo-2499/.hgsub
>> +  [255]
>> +  $ hg status
>> +  ! .hgsub
>> +  ! .hgsubstate
>> +  $ hg cat -r1 .hgsub > .hgsub
>> +  $ hg revert --no-backup .hgsubstate
> 
> Urgh, the abort does not look good. 
> 
> I mean, if you do something like:
> 
> $ touch foo
> $ hg qnew some-patch
> $ hg add foo; hg qref foo
> $ rm foo
> $ hg qref
> 
> It does not fail, so I would expect the same behaviour here.

I initially thought the same, but if you do:

$ hg init sub
$ (cd sub && echo a > a && hg ci -Am0sub)
$ echo sub = sub > .hgsub
$ hg ci -Am0
$ rm .hgsub
$ hg ci -mfoo

you get the same "abort: No such file or directory: .hgsub" and a broken state. It's a pathological case anyway—one shouldn't go around deleting one's .hgsub file.

> Questions:
> 1) what happens if .hgsub(state) has pending changes, and if we try a qpop/qpush?
> Usually, those commands abort when we have local changes; if we decide to ignore .hgsub(state) we should not abort, and make sure that the files are untouched while qpopping or qpushing.

Right now qpop/qpush abort. I figured this was the right option—otherwise you can create a state where your code in the patch queue depends on a subrepo, but when you qfinish the patch queue the subrepo isn't represented there.

> 2) Similarly, what happens if you qimport a patch that has .hgsub(state) changes? I agree, maybe this usecase is rare. We could decide to ignore it as a particularly silly example. But I still think that when qpushing a patch, we may want to check that we never touch .hgsub(state)?

There's a similar problem that I noticed after I patchbombed this series—qnew -f also allows you to bypass the checks and include .hgsub{,state}, since it doesn't go through queue.refresh(). My goal was to make it as close to impossible as I could to get a patch into the queue that changes .hgsub{,state}.

I fear doing it properly might require a non-trivial refactoring of mq.py though. I was looking for ways to factor out the file selection and patch hunk writing of queue.new() and queue.refresh(), but didn't get too far.

pacem in terris / mir / shanti / salaam / heiwa
Kevin R. Bullock