Patchwork [2,of,2] summary: don't indicate a clean wdir() with deleted files (issue5549)

login
register
mail settings
Submitter Matt Harbison
Date July 3, 2017, 2:50 a.m.
Message ID <fcbab9a85517f363a9c0.1499050221@Envy>
Download mbox | patch
Permalink /patch/21944/
State Accepted
Headers show

Comments

Matt Harbison - July 3, 2017, 2:50 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1498705338 14400
#      Wed Jun 28 23:02:18 2017 -0400
# Node ID fcbab9a85517f363a9c0bdfa80362b88843258ac
# Parent  0fcbcb681899ad44a33380788750ea375aa70864
summary: don't indicate a clean wdir() with deleted files (issue5549)

It wasn't a BC when copy/rename was added (997ab9af81df), or when subrepos were
added (c4347e48b0d0).
Yuya Nishihara - July 3, 2017, 1:59 p.m.
On Sun, 02 Jul 2017 22:50:21 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1498705338 14400
> #      Wed Jun 28 23:02:18 2017 -0400
> # Node ID fcbab9a85517f363a9c0bdfa80362b88843258ac
> # Parent  0fcbcb681899ad44a33380788750ea375aa70864
> summary: don't indicate a clean wdir() with deleted files (issue5549)
> 
> It wasn't a BC when copy/rename was added (997ab9af81df), or when subrepos were
> added (c4347e48b0d0).

(This changes the existing behavior, so should be marked as BC, I think.)

> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4870,8 +4870,8 @@
>      elif (parents[0].closesbranch() and
>            pnode in repo.branchheads(branch, closed=True)):
>          t += _(' (head closed)')
> -    elif not (status.modified or status.added or status.removed or renamed or
> -              copied or subs):
> +    elif not (status.modified or status.added or status.removed or
> +              status.deleted or renamed or copied or subs):
>          t += _(' (clean)')
>          cleanworkdir = True

This seemed better to me, but I guess the original intent was to show whether
new commit could be made (=not clean) or not (=clean).

> @@ -35,7 +35,7 @@
>    parent: 3:cb66ec850af7 tip
>     add b
>    branch: default
> -  commit: 1 deleted (clean)
> +  commit: 1 deleted

Probably that's why this field is called "commit".

Thoughts?
Katsunori FUJIWARA - July 3, 2017, 3:40 p.m.
At Mon, 3 Jul 2017 22:59:38 +0900,
Yuya Nishihara wrote:
> 
> On Sun, 02 Jul 2017 22:50:21 -0400, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1498705338 14400
> > #      Wed Jun 28 23:02:18 2017 -0400
> > # Node ID fcbab9a85517f363a9c0bdfa80362b88843258ac
> > # Parent  0fcbcb681899ad44a33380788750ea375aa70864
> > summary: don't indicate a clean wdir() with deleted files (issue5549)
> > 
> > It wasn't a BC when copy/rename was added (997ab9af81df), or when subrepos were
> > added (c4347e48b0d0).
> 
> (This changes the existing behavior, so should be marked as BC, I think.)
> 
> > 
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -4870,8 +4870,8 @@
> >      elif (parents[0].closesbranch() and
> >            pnode in repo.branchheads(branch, closed=True)):
> >          t += _(' (head closed)')
> > -    elif not (status.modified or status.added or status.removed or renamed or
> > -              copied or subs):
> > +    elif not (status.modified or status.added or status.removed or
> > +              status.deleted or renamed or copied or subs):
> >          t += _(' (clean)')
> >          cleanworkdir = True
> 
> This seemed better to me, but I guess the original intent was to show whether
> new commit could be made (=not clean) or not (=clean).

Is there any short phrase corresponding to "new commit could be made" ?
It seems better than "clean" in this case.

> > @@ -35,7 +35,7 @@
> >    parent: 3:cb66ec850af7 tip
> >     add b
> >    branch: default
> > -  commit: 1 deleted (clean)
> > +  commit: 1 deleted
> 
> Probably that's why this field is called "commit".
> 
> Thoughts?
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Harbison - July 4, 2017, 2:30 a.m.
On Mon, 03 Jul 2017 09:59:38 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 02 Jul 2017 22:50:21 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1498705338 14400
>> #      Wed Jun 28 23:02:18 2017 -0400
>> # Node ID fcbab9a85517f363a9c0bdfa80362b88843258ac
>> # Parent  0fcbcb681899ad44a33380788750ea375aa70864
>> summary: don't indicate a clean wdir() with deleted files (issue5549)
>>
>> It wasn't a BC when copy/rename was added (997ab9af81df), or when  
>> subrepos were
>> added (c4347e48b0d0).
>
> (This changes the existing behavior, so should be marked as BC, I think.)
>
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -4870,8 +4870,8 @@
>>      elif (parents[0].closesbranch() and
>>            pnode in repo.branchheads(branch, closed=True)):
>>          t += _(' (head closed)')
>> -    elif not (status.modified or status.added or status.removed or  
>> renamed or
>> -              copied or subs):
>> +    elif not (status.modified or status.added or status.removed or
>> +              status.deleted or renamed or copied or subs):
>>          t += _(' (clean)')
>>          cleanworkdir = True
>
> This seemed better to me, but I guess the original intent was to show  
> whether
> new commit could be made (=not clean) or not (=clean).
>
>> @@ -35,7 +35,7 @@
>>    parent: 3:cb66ec850af7 tip
>>     add b
>>    branch: default
>> -  commit: 1 deleted (clean)
>> +  commit: 1 deleted
>
> Probably that's why this field is called "commit".
>
> Thoughts?

Yeah, I kind of wondered if that's what was going on, and why I went back  
and forth on it for the last 2.5 years.  It came up when writing the  
training for new developers, and people understandably associated "clean  
working directory" with this line.

I'm fine with making the presence of '+' in `hg identify` output be the  
way to tell dirty or not, as long as '!' in a subrepo triggers a '+' on  
the top level repo.  (Though it seems like there could be an explicit line  
and text in `hg summary` too.)  Maybe to alleviate the confusion, change  
'clean' here to 'uncommittable'?  Or 'nothing', to be less scary?  (Though  
at that point we've broken anything parsing it, so IDK if slightly  
changing the meaning matters too much.)

I also think that the --check flag needs to be fixed up too.
Yuya Nishihara - July 4, 2017, 1:10 p.m.
On Mon, 03 Jul 2017 22:30:34 -0400, Matt Harbison wrote:
> I'm fine with making the presence of '+' in `hg identify` output be the  
> way to tell dirty or not, as long as '!' in a subrepo triggers a '+' on  
> the top level repo.

Agreed. sub.dirty() would need "missing" flag as ctx.dirty() has.

> (Though it seems like there could be an explicit line  
> and text in `hg summary` too.)

Perhaps.

>  Maybe to alleviate the confusion, change  
> 'clean' here to 'uncommittable'?  Or 'nothing', to be less scary?  (Though  
> at that point we've broken anything parsing it, so IDK if slightly  
> changing the meaning matters too much.)

Not sure if 'uncommittable' is the right word, which sounds like committing
is proactively disabled. 'nothing' is slightly obscure, but seems better than
'uncommittable'.

> I also think that the --check flag needs to be fixed up too.

--check for deleted files in subrepos? Perhaps.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4870,8 +4870,8 @@ 
     elif (parents[0].closesbranch() and
           pnode in repo.branchheads(branch, closed=True)):
         t += _(' (head closed)')
-    elif not (status.modified or status.added or status.removed or renamed or
-              copied or subs):
+    elif not (status.modified or status.added or status.removed or
+              status.deleted or renamed or copied or subs):
         t += _(' (clean)')
         cleanworkdir = True
     elif pnode not in bheads:
diff --git a/tests/test-merge-subrepos.t b/tests/test-merge-subrepos.t
--- a/tests/test-merge-subrepos.t
+++ b/tests/test-merge-subrepos.t
@@ -35,7 +35,7 @@ 
   parent: 3:cb66ec850af7 tip
    add b
   branch: default
-  commit: 1 deleted (clean)
+  commit: 1 deleted
   update: 1 new changesets, 2 branch heads (merge)
   phases: 4 draft