Patchwork [1,of,4] context.status: wipe deleted/unknown/ignored fields when reversed

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 13, 2014, 6:29 a.m.
Message ID <4efe64f7b1b244f9224a.1415860193@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6699/
State Accepted
Headers show

Comments

Martin von Zweigbergk - Nov. 13, 2014, 6:29 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1415855947 28800
#      Wed Nov 12 21:19:07 2014 -0800
# Node ID 4efe64f7b1b244f9224a0706bb98f358e24e3dd3
# Parent  3480c07fc934a9c90c8dda1ed2bc98fd9c96c44a
context.status: wipe deleted/unknown/ignored fields when reversed

It makes no sense to request reverse status (i.e. changes from the
working copy to its parent) and then look at the deleted, unknown or
ignored fields. If you do, you would get the result from the forward
status (changes from parent to the working copy). Instead of giving a
nonsensical answer to a nonsensical question, it seems a little saner
to return empty lists. It might be best if we could prevent the caller
accessing these lists, but it's doubtful it's worth the trouble.
Pierre-Yves David - Nov. 13, 2014, 5:32 p.m.
On 11/13/2014 06:29 AM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1415855947 28800
> #      Wed Nov 12 21:19:07 2014 -0800
> # Node ID 4efe64f7b1b244f9224a0706bb98f358e24e3dd3
> # Parent  3480c07fc934a9c90c8dda1ed2bc98fd9c96c44a
> context.status: wipe deleted/unknown/ignored fields when reversed
>
> It makes no sense to request reverse status (i.e. changes from the
> working copy to its parent) and then look at the deleted, unknown or
> ignored fields. If you do, you would get the result from the forward
> status (changes from parent to the working copy). Instead of giving a
> nonsensical answer to a nonsensical question, it seems a little saner
> to return empty lists.


> It might be best if we could prevent the caller
> accessing these lists, but it's doubtful it's worth the trouble.

I think it worth the trouble and would sleep better at night if we did.
Martin von Zweigbergk - Nov. 13, 2014, 8:55 p.m.
On Thu Nov 13 2014 at 9:32:41 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/13/2014 06:29 AM, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1415855947 28800
> > #      Wed Nov 12 21:19:07 2014 -0800
> > # Node ID 4efe64f7b1b244f9224a0706bb98f358e24e3dd3
> > # Parent  3480c07fc934a9c90c8dda1ed2bc98fd9c96c44a
> > context.status: wipe deleted/unknown/ignored fields when reversed
> >
> > It makes no sense to request reverse status (i.e. changes from the
> > working copy to its parent) and then look at the deleted, unknown or
> > ignored fields. If you do, you would get the result from the forward
> > status (changes from parent to the working copy). Instead of giving a
> > nonsensical answer to a nonsensical question, it seems a little saner
> > to return empty lists.
>
>
> > It might be best if we could prevent the caller
> > accessing these lists, but it's doubtful it's worth the trouble.
>
> I think it worth the trouble and would sleep better at night if we did.
>

I'll send a new version where I use None instead. That prevents iteration
over the files. Completely preventing access seems harder, but I'm open to
suggestions.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -304,9 +304,12 @@ 
         r = ctx2._buildstatus(ctx1, r, match, listignored, listclean,
                               listunknown)
 
+        r = scmutil.status(*r)
         if reversed:
-            # reverse added and removed
-            r[1], r[2] = r[2], r[1]
+            # Reverse added and removed. Clear deleted, unknown and ignored as
+            # these make no sense to reverse.
+            r = scmutil.status(r.modified, r.removed, r.added, [], [], [],
+                               r.clean)
 
         if listsubrepos:
             for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
@@ -325,8 +328,7 @@ 
         for l in r:
             l.sort()
 
-        # we return a tuple to signify that this list isn't changing
-        return scmutil.status(*r)
+        return r
 
 
 def makememctx(repo, parents, text, user, date, branch, files, store,