[PATCH 1 of 3] subrepo: fix diff/status -S of added subrepo (issue3056)

[Matt Mackall]

On Thu, 2012-05-17 at 00:46 +0300, Dov Feldstern wrote:
> # HG changeset patch
> # User Dov Feldstern <dfeldstern at gmail.com>
> # Date 1337203115 -10800
> # Branch stable
> # Node ID 52df9ecdec9b77cd3b333f4505cf14b0f22f230c
> # Parent  7002bb17cc5eceb8c1ab792bdaf10bd96c08c8d8
> subrepo: fix diff/status -S of added subrepo (issue3056)
> 
> When comparing revisions between which a subrepo was added (i.e., subrepo
> doesn't yet exist in rev1, but does exist in rev2), diff/status -S should
> report the new subrepo's files as newly added files (because, from the parent
> repo's point of view, the contents of the new subrepo is new content). However,
> until now, the subrepo's contents was not reported at all in the comparison of
> rev1 and rev2.
> 
> The problem was due to the fact that when iterating over the subrepos in the
> two revisions being compared, if at a given subpath there was no subrepo in
> rev1, then we would return the subrepo for that subpath in rev2. This, however,
> does not allow us to consistently show the differences going from rev1 -> rev2,
> because we are now left with a set of subrepos which are from either rev1 or
> rev2,and we have no way of knowing which it is.
> 
> The solution is that in the above case, we should use rev2 as a "helper
> context" for deciding what *kind* of repository to return (we need to return a
> subrepo, so that we will then be able to compare it with the matching subrepo
> from rev2); however, the returned subrepo should be in the null state (which
> reflects the fact that the subrepo didn't yet exist in rev1). Then, it can be
> compared with the matching subrepo from rev2, and the comparison is still being
> done in the right direction (rev1 -> rev2).
> 
> Note that this changeset only solves half of the issue --- the direction in
> which subrepo was *added* between rev1 and rev2. The other direction (subrepo
> *removed* between rev1 and rev2) will be treated in the next changeset.
> 
> This changeset also fixes some existing tests which until now were displaying
> the incorrect behavior.
> 
> diff -r 7002bb17cc5e -r 52df9ecdec9b mercurial/context.py
> --- a/mercurial/context.py	Mon May 14 13:25:42 2012 +0100
> +++ b/mercurial/context.py	Thu May 17 00:18:35 2012 +0300
> @@ -290,8 +290,11 @@
>              if match.bad(fn, _('no such file in rev %s') % self) and match(fn):
>                  yield fn
>  
> -    def sub(self, path):
> -        return subrepo.subrepo(self, path)
> +    def sub(self, path, helperctx=None):
> +        '''return subrepo at path. If there is no subrepo at path, but
> +        there is a subrepo at path in the provided helperctx, return
> +        a subrepo of the type in helperctx, set to the null state.'''
> +        return subrepo.subrepo(self, path, helperctx)

Not super-excited about this optional keyword arg approach. I think I'd
rather see something like:

- make subrepo.subrepo(<not a subrepo path>) fail
- provide an explicit "null constructor"
- make itersubrepos take one arg and be boring
- make a deltasubrepos that takes two args and is suitable for diff
- teach deltasubrepos to use the null constructor when appropriate
- use deltasubrepos where appropriate
- reorder the above steps in the obvious way that doesn't break at each
step

How does that sound?

-- 
Mathematics is the supreme nostalgia of our time.