Bug 2499 - MQ allows creation of inconsistent subrepo state
Summary: MQ allows creation of inconsistent subrepo state
Status: RESOLVED FIXED
Alias: None
Product: Mercurial
Classification: Unclassified
Component: Mercurial (show other bugs)
Version: unspecified
Hardware: All All
: normal bug
Assignee: Kevin Bullock
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-15 22:45 UTC by Kevin Bullock
Modified: 2011-01-12 09:53 UTC (History)
6 users (show)

See Also:
Python Version: ---


Attachments
(34 bytes, application/x-sh)
2010-11-15 22:45 UTC, Kevin Bullock
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Bullock 2010-11-15 22:45 UTC
Using MQ to create a patch that adds a subrepo can create an inconsistent state 
from which it's difficult to recover. `hg qrefresh` doesn't do a recursive commit 
(which I believe is reasonable), but this fact makes it easy to commit a 
changeset with an addition to .hgsub, but no matching entry in .hgsubstate.

If there isn't an existing .hgsubstate file, this doesn't seem to cause any 
trouble, but when there is (i.e. you had already added one subrepo, and are 
adding another), `hg up null && hg up` aborts with:

abort: 00changelog.i@: ambiguous identifier!

I've attached a test script that reproduces the problem. The script creates an 
empty .hgsubstate file to represent a previously-existing .hgsubstate file for 
another subrepo.

I'm going to work on a fix for this (admittedly hairy) use case. I'm thinking 
that there should be a way to handle it gracefully _with_ an existing .hgsubstate 
file as well as without, but I think MQ should also detect changes to .hgsub and 
either (a) warn on qrefresh or (b) do a recursive commit on qfinish. I'd 
appreciate advice on how to proceed.
Comment 1 Matt Mackall 2010-11-15 23:30 UTC
Subrepos are not defined to be Mercurial repos. So qpop can not, in general,
be implemented on them. mq should probably either ignore subrepos or refuse
to operate on dirty ones.
Comment 2 Kevin Bullock 2010-11-15 23:48 UTC
So the difference between having a blank .hgsubstate and not having it at all 
is that subrepo.submerge() never gets invoked in the former case, and thus no 
attempt is made to update the subrepo. Is this a bug in itself? Shouldn't we 
print an error if there's an entry in the _committed_ .hgsub file that has no 
corresponding .hgsubstate entry?
Comment 3 Kevin Bullock 2010-11-15 23:59 UTC
@mpm I can't say if there's related issues with qpush, qpop, etc. not checking 
the subrepo state; this one is really just how MQ handles (or doesn't handle) 
changes made to .hgsub in a patch. Your comment would seem to suggest, though, 
that updating .hgsubstate on qfinish of a patch that modifies .hgsub would not 
be a great option?
Comment 4 Matt Mackall 2010-11-16 00:02 UTC
I'm suggesting that MQ should refuse to work with subrepos, period. There's
no such thing as qpop on a subrepo, so you can't do mq on a subrepo, so
you'd better not mess with them at all. That means no tracking of changes to
.hgsub*.
Comment 5 Patrick Mézard 2010-11-17 06:16 UTC
"There's no such thing as qpop on a subrepo, so you can't do mq on a
subrepo, so you'd better not mess with them at all. That means no tracking
of changes to .hgsub*."

I agree with the first sentence but not with the conclusion.

Here is my use case: I work with "proj" which depends on "lib". I want to
implement a new feature in proj which requires a more recent lib. I update
lib revision (but lib is clean), work on my feature. At this point I want to
qnew and switch back to something else.

This case seems perfectly legit and in fact I do that daily with
hgsubversion which has something a similar to subrepo to handle
svn:externals. There is no problem with MQ because:
- The externals state is completely defined in .hgsvnexternals, including
the target revision. After editing it, users have to explicitely update the
externals using another command. hgsubversion does not automatically record
stuff on commits.
- We do not care about externals dirtiness.

Instead of preventing MQ to handle subrepos I suggest that:
- MQ aborts if any subrepo is dirty before creating/amending a patch
- MQ updates .hgsubstate when creating/amending a patch
Comment 6 HG Bot 2010-11-22 14:00 UTC
Fixed by http://selenic.com/repo/hg/rev/f08df4d38442
Kevin Bullock <kbullock@ringworld.org>
mq: ignore subrepos (issue2499)

(please test the fix)
Comment 7 Kevin Bullock 2010-11-22 14:06 UTC
Re-marking as in-progress since there are more ways than qrefresh that can 
introduce modifications to .hgsub{,state} into a patch (namely qnew and 
qrecord).
Comment 8 HG Bot 2010-12-20 13:00 UTC
Fixed by http://selenic.com/repo/hg/rev/be7e8e9bc5e5
Kevin Bullock <kbullock@ringworld.org>
mq: update .hgsubstate if subrepos are clean (issue2499)

(please test the fix)
Comment 9 Shawn Hoover 2011-01-04 09:49 UTC
Has this been tested with svn subrepos? I suspected the qpush/qpop test 
would fail when checking `hg status -AS` after qpop (svn working copies are 
not implicitly ignored like hg subrepos are), but when I run the commands I 
get an error even before that.

>hg init
>svn co http://localhost/svn/sub
A    a
 U   sub
Checked out revision 17689.
>echo sub = [svn]http://localhost/svn/sub > .hgsub
>hg add .hgsub
>hg qnew -m0 0.diff
abort: uncommitted changes in subrepository inc
>cd sub
>svn st
>
Comment 10 Kevin Bullock 2011-01-04 11:18 UTC
I've verified the issue with svn subrepos. Minor thinko in the ignoreupdate 
check in svnsubrepo.dirty(). Patch with tests forthcoming.
Comment 11 HG Bot 2011-01-11 15:00 UTC
Fixed by http://selenic.com/repo/hg/rev/a8cef95cea88
Kevin Bullock <kbullock@ringworld.org>
subrepo: fix svnsubrepo.dirty() checking of ignoreupdate (issue2499)

(please test the fix)
Comment 12 Shawn Hoover 2011-01-12 09:49 UTC
Thanks, that test passes now.
Comment 13 Patrick Mézard 2011-01-12 09:53 UTC
In main, resolving
Comment 14 Bugzilla 2012-05-12 09:14 UTC

--- Bug imported by bugzilla@serpentine.com 2012-05-12 09:14 EDT  ---

This bug was previously known as _bug_ 2499 at http://mercurial.selenic.com/bts/issue2499
Imported an attachment (id=1482)