Patchwork [2,of,5] test-revert: don't unnecessarily pipe through 'cat'

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 2, 2014, 10:19 p.m.
Message ID <9439327a222474b6af32.1414966770@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6549/
State Accepted
Headers show

Comments

Martin von Zweigbergk - Nov. 2, 2014, 10:19 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1413682221 25200
#      Sat Oct 18 18:30:21 2014 -0700
# Branch stable
# Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
# Parent  5df55ad6918a76870695dfe855168309bb5da27c
test-revert: don't unnecessarily pipe through 'cat'
Pierre-Yves David - Nov. 2, 2014, 10:45 p.m.
On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1413682221 25200
> #      Sat Oct 18 18:30:21 2014 -0700
> # Branch stable
> # Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
> # Parent  5df55ad6918a76870695dfe855168309bb5da27c
> test-revert: don't unnecessarily pipe through 'cat'
>
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -730,7 +730,7 @@
>
>   Setup working directory
>
> -  $ python ../gen-revert-cases.py wc | cat
> +  $ python ../gen-revert-cases.py wc
>     $ hg addremove --similarity 0
>     removing added_removed
>     removing added_revert


check-code says hi:

    Skipping mercurial/httpclient/socketutil.py it has no-che?k-code (glob)
+  tests/test-revert.t:733:
+   >   $ python ../gen-revert-cases.py wc
+   filter wc output
+  [1]


I'm not sure about the 3 others patches. What is wrong with some 
duplicated case? Why is it worth introducing special casing to remove them ?
Martin von Zweigbergk - Nov. 2, 2014, 11:05 p.m.
On Sun Nov 02 2014 at 2:45:40 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1413682221 25200
> > #      Sat Oct 18 18:30:21 2014 -0700
> > # Branch stable
> > # Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
> > # Parent  5df55ad6918a76870695dfe855168309bb5da27c
> > test-revert: don't unnecessarily pipe through 'cat'
> >
> > diff --git a/tests/test-revert.t b/tests/test-revert.t
> > --- a/tests/test-revert.t
> > +++ b/tests/test-revert.t
> > @@ -730,7 +730,7 @@
> >
> >   Setup working directory
> >
> > -  $ python ../gen-revert-cases.py wc | cat
> > +  $ python ../gen-revert-cases.py wc
> >     $ hg addremove --similarity 0
> >     removing added_removed
> >     removing added_revert
>
>
> check-code says hi:
>
>     Skipping mercurial/httpclient/socketutil.py it has no-che?k-code
> (glob)
> +  tests/test-revert.t:733:
> +   >   $ python ../gen-revert-cases.py wc
> +   filter wc output
> +  [1]
>

Oops, sorry about that. What's the purpose of it? The check was in the
first version of check-code, so I couldn't find any explanation for it in
the changelog.


> I'm not sure about the 3 others patches. What is wrong with some
> duplicated case? Why is it worth introducing special casing to remove them
> ?
>

I think the file histories are clearer when expressed in terms of the
states rather than transitions, so in the end (after ~7 more patches), e.g.
'modified_untracked-clean' becomes 'content1_content2_untracked_content2'.
The generation script actually becomes a bit simpler at that point. I
suppose a 11-patch series is too long. Do you want to see the end state in
some other form before you decide?
Pierre-Yves David - Nov. 3, 2014, 1:20 p.m.
On 11/02/2014 11:05 PM, Martin von Zweigbergk wrote:
>
>
> On Sun Nov 02 2014 at 2:45:40 PM Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
>      > # HG changeset patch
>      > # User Martin von Zweigbergk <martinvonz@google.com
>     <mailto:martinvonz@google.com>>
>      > # Date 1413682221 25200
>      > #      Sat Oct 18 18:30:21 2014 -0700
>      > # Branch stable
>      > # Node ID 9439327a222474b6af32ed9f717bfc__e81f90e53b
>      > # Parent  5df55ad6918a76870695dfe8551683__09bb5da27c
>      > test-revert: don't unnecessarily pipe through 'cat'
>      >
>      > diff --git a/tests/test-revert.t b/tests/test-revert.t
>      > --- a/tests/test-revert.t
>      > +++ b/tests/test-revert.t
>      > @@ -730,7 +730,7 @@
>      >
>      >   Setup working directory
>      >
>      > -  $ python ../gen-revert-cases.py wc | cat
>      > +  $ python ../gen-revert-cases.py wc
>      >     $ hg addremove --similarity 0
>      >     removing added_removed
>      >     removing added_revert
>
>
>     check-code says hi:
>
>          Skipping mercurial/httpclient/__socketutil.py it has
>     no-che?k-code (glob)
>     +  tests/test-revert.t:733:
>     +   >   $ python ../gen-revert-cases.py wc
>     +   filter wc output
>     +  [1]
>
>
> Oops, sorry about that. What's the purpose of it? The check was in the
> first version of check-code, so I couldn't find any explanation for it
> in the changelog.

I don't known why it is here. We should probably ask Matt about it (cced 
him)

>     I'm not sure about the 3 others patches. What is wrong with some
>     duplicated case? Why is it worth introducing special casing to
>     remove them ?
>
>
> I think the file histories are clearer when expressed in terms of the
> states rather than transitions, so in the end (after ~7 more patches),
> e.g. 'modified_untracked-clean' becomes
> 'content1_content2_untracked_content2'. The generation script actually
> becomes a bit simpler at that point. I suppose a 11-patch series is too
> long. Do you want to see the end state in some other form before you decide?

You should include this kind of high level goal in your commit message:

   I'm doing XXX because we want to be able to do YYY in the future. The 
change is correct because YYY

This seems a reasonable changes, but it makes is a bit (but just a bit) 
harder to compute the expected revert action from the name (since you 
have to make the diff yourself).

I'm curious about the whole series. Can you push to non publishing repo 
for me to have a look ?

(note, your naming scheme should make a clearer distinction between each 
part, eg: content1_content2_untracked-content2)
Mads Kiilerich - Nov. 3, 2014, 2:53 p.m.
On 11/03/2014 02:20 PM, Pierre-Yves David wrote:
>
>
> On 11/02/2014 11:05 PM, Martin von Zweigbergk wrote:
>>
>>
>> On Sun Nov 02 2014 at 2:45:40 PM Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
>>      > # HG changeset patch
>>      > # User Martin von Zweigbergk <martinvonz@google.com
>>     <mailto:martinvonz@google.com>>
>>      > # Date 1413682221 25200
>>      > #      Sat Oct 18 18:30:21 2014 -0700
>>      > # Branch stable
>>      > # Node ID 9439327a222474b6af32ed9f717bfc__e81f90e53b
>>      > # Parent  5df55ad6918a76870695dfe8551683__09bb5da27c
>>      > test-revert: don't unnecessarily pipe through 'cat'
>>      >
>>      > diff --git a/tests/test-revert.t b/tests/test-revert.t
>>      > --- a/tests/test-revert.t
>>      > +++ b/tests/test-revert.t
>>      > @@ -730,7 +730,7 @@
>>      >
>>      >   Setup working directory
>>      >
>>      > -  $ python ../gen-revert-cases.py wc | cat
>>      > +  $ python ../gen-revert-cases.py wc
>>      >     $ hg addremove --similarity 0
>>      >     removing added_removed
>>      >     removing added_revert
>>
>>
>>     check-code says hi:
>>
>>          Skipping mercurial/httpclient/__socketutil.py it has
>>     no-che?k-code (glob)
>>     +  tests/test-revert.t:733:
>>     +   >   $ python ../gen-revert-cases.py wc
>>     +   filter wc output
>>     +  [1]
>>
>>
>> Oops, sorry about that. What's the purpose of it? The check was in the
>> first version of check-code, so I couldn't find any explanation for it
>> in the changelog.
>
> I don't known why it is here. We should probably ask Matt about it 
> (cced him)

IIRC, the output of wc varies across platforms.

In this case you could use another name than wc or change the check to 
look for something like   (^|\|\s*)\bwc\b

/Mads
Martin von Zweigbergk - Nov. 3, 2014, 4:05 p.m.
Oh! It never even hit me that it was about the command. Thanks for
explaining. I don't think I'll bother changing it, but I'm glad I know what
the issue was about.

On Mon, Nov 3, 2014, 06:53 Mads Kiilerich <mads@kiilerich.com> wrote:

> On 11/03/2014 02:20 PM, Pierre-Yves David wrote:
> >
> >
> > On 11/02/2014 11:05 PM, Martin von Zweigbergk wrote:
> >>
> >>
> >> On Sun Nov 02 2014 at 2:45:40 PM Pierre-Yves David
> >> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org
> >>
> >> wrote:
> >>
> >>
> >>
> >>     On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
> >>      > # HG changeset patch
> >>      > # User Martin von Zweigbergk <martinvonz@google.com
> >>     <mailto:martinvonz@google.com>>
> >>      > # Date 1413682221 25200
> >>      > #      Sat Oct 18 18:30:21 2014 -0700
> >>      > # Branch stable
> >>      > # Node ID 9439327a222474b6af32ed9f717bfc__e81f90e53b
> >>      > # Parent  5df55ad6918a76870695dfe8551683__09bb5da27c
> >>      > test-revert: don't unnecessarily pipe through 'cat'
> >>      >
> >>      > diff --git a/tests/test-revert.t b/tests/test-revert.t
> >>      > --- a/tests/test-revert.t
> >>      > +++ b/tests/test-revert.t
> >>      > @@ -730,7 +730,7 @@
> >>      >
> >>      >   Setup working directory
> >>      >
> >>      > -  $ python ../gen-revert-cases.py wc | cat
> >>      > +  $ python ../gen-revert-cases.py wc
> >>      >     $ hg addremove --similarity 0
> >>      >     removing added_removed
> >>      >     removing added_revert
> >>
> >>
> >>     check-code says hi:
> >>
> >>          Skipping mercurial/httpclient/__socketutil.py it has
> >>     no-che?k-code (glob)
> >>     +  tests/test-revert.t:733:
> >>     +   >   $ python ../gen-revert-cases.py wc
> >>     +   filter wc output
> >>     +  [1]
> >>
> >>
> >> Oops, sorry about that. What's the purpose of it? The check was in the
> >> first version of check-code, so I couldn't find any explanation for it
> >> in the changelog.
> >
> > I don't known why it is here. We should probably ask Matt about it
> > (cced him)
>
> IIRC, the output of wc varies across platforms.
>
> In this case you could use another name than wc or change the check to
> look for something like   (^|\|\s*)\bwc\b
>
> /Mads
>
Matt Mackall - Nov. 3, 2014, 5 p.m.
On Sun, 2014-11-02 at 22:45 +0000, Pierre-Yves David wrote:
> 
> On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1413682221 25200
> > #      Sat Oct 18 18:30:21 2014 -0700
> > # Branch stable
> > # Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
> > # Parent  5df55ad6918a76870695dfe855168309bb5da27c
> > test-revert: don't unnecessarily pipe through 'cat'
> >
> > diff --git a/tests/test-revert.t b/tests/test-revert.t
> > --- a/tests/test-revert.t
> > +++ b/tests/test-revert.t
> > @@ -730,7 +730,7 @@
> >
> >   Setup working directory
> >
> > -  $ python ../gen-revert-cases.py wc | cat
> > +  $ python ../gen-revert-cases.py wc
> >     $ hg addremove --similarity 0
> >     removing added_removed
> >     removing added_revert
> 
> 
> check-code says hi:
> 
>     Skipping mercurial/httpclient/socketutil.py it has no-che?k-code (glob)
> +  tests/test-revert.t:733:
> +   >   $ python ../gen-revert-cases.py wc
> +   filter wc output
> +  [1]

$ hg blame test-revert.t | grep "| cat"
pierre-yves:   $ python ../gen-revert-cases.py wc | cat

Ahem.

http://mercurial.selenic.com/wiki/WritingTests#wc
Pierre-Yves David - Nov. 3, 2014, 5:04 p.m.
On 11/03/2014 05:00 PM, Matt Mackall wrote:
> On Sun, 2014-11-02 at 22:45 +0000, Pierre-Yves David wrote:
>>
>> On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> # Date 1413682221 25200
>>> #      Sat Oct 18 18:30:21 2014 -0700
>>> # Branch stable
>>> # Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
>>> # Parent  5df55ad6918a76870695dfe855168309bb5da27c
>>> test-revert: don't unnecessarily pipe through 'cat'
>>>
>>> diff --git a/tests/test-revert.t b/tests/test-revert.t
>>> --- a/tests/test-revert.t
>>> +++ b/tests/test-revert.t
>>> @@ -730,7 +730,7 @@
>>>
>>>    Setup working directory
>>>
>>> -  $ python ../gen-revert-cases.py wc | cat
>>> +  $ python ../gen-revert-cases.py wc
>>>      $ hg addremove --similarity 0
>>>      removing added_removed
>>>      removing added_revert
>>
>>
>> check-code says hi:
>>
>>      Skipping mercurial/httpclient/socketutil.py it has no-che?k-code (glob)
>> +  tests/test-revert.t:733:
>> +   >   $ python ../gen-revert-cases.py wc
>> +   filter wc output
>> +  [1]
>
> $ hg blame test-revert.t | grep "| cat"
> pierre-yves:   $ python ../gen-revert-cases.py wc | cat
>
> Ahem.

The question was about why we catch the wc binary in test. I think mads 
give a insighful reply

> http://mercurial.selenic.com/wiki/WritingTests#wc

And this adds some more.
Martin von Zweigbergk - Nov. 3, 2014, 6 p.m.
On Mon Nov 03 2014 at 5:20:28 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/02/2014 11:05 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Sun Nov 02 2014 at 2:45:40 PM Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> > wrote:
> >
> >
> >
> >     On 11/02/2014 10:19 PM, Martin von Zweigbergk wrote:
> >      > # HG changeset patch
> >      > # User Martin von Zweigbergk <martinvonz@google.com
> >     <mailto:martinvonz@google.com>>
> >      > # Date 1413682221 25200
> >      > #      Sat Oct 18 18:30:21 2014 -0700
> >      > # Branch stable
> >      > # Node ID 9439327a222474b6af32ed9f717bfc__e81f90e53b
> >      > # Parent  5df55ad6918a76870695dfe8551683__09bb5da27c
> >      > test-revert: don't unnecessarily pipe through 'cat'
> >      >
> >      > diff --git a/tests/test-revert.t b/tests/test-revert.t
> >      > --- a/tests/test-revert.t
> >      > +++ b/tests/test-revert.t
> >      > @@ -730,7 +730,7 @@
> >      >
> >      >   Setup working directory
> >      >
> >      > -  $ python ../gen-revert-cases.py wc | cat
> >      > +  $ python ../gen-revert-cases.py wc
> >      >     $ hg addremove --similarity 0
> >      >     removing added_removed
> >      >     removing added_revert
> >
> >
> >     check-code says hi:
> >
> >          Skipping mercurial/httpclient/__socketutil.py it has
> >     no-che?k-code (glob)
> >     +  tests/test-revert.t:733:
> >     +   >   $ python ../gen-revert-cases.py wc
> >     +   filter wc output
> >     +  [1]
> >
> >
> > Oops, sorry about that. What's the purpose of it? The check was in the
> > first version of check-code, so I couldn't find any explanation for it
> > in the changelog.
>
> I don't known why it is here. We should probably ask Matt about it (cced
> him)
>
> >     I'm not sure about the 3 others patches. What is wrong with some
> >     duplicated case? Why is it worth introducing special casing to
> >     remove them ?
> >
> >
> > I think the file histories are clearer when expressed in terms of the
> > states rather than transitions, so in the end (after ~7 more patches),
> > e.g. 'modified_untracked-clean' becomes
> > 'content1_content2_untracked_content2'. The generation script actually
> > becomes a bit simpler at that point. I suppose a 11-patch series is too
> > long. Do you want to see the end state in some other form before you
> decide?
>
> You should include this kind of high level goal in your commit message:
>
>    I'm doing XXX because we want to be able to do YYY in the future. The
> change is correct because YYY
>

I put the motivation in the first patch now instead of the main one. Thanks.


> This seems a reasonable changes, but it makes is a bit (but just a bit)
> harder to compute the expected revert action from the name (since you
> have to make the diff yourself).
>

> I'm curious about the whole series. Can you push to non publishing repo
> for me to have a look ?
>

Available at
https://bitbucket.org/martinvonz/hg/branch/stable?head=e8138b5aaaf96501827715c89144367d9622eb3b


>
> (note, your naming scheme should make a clearer distinction between each
> part, eg: content1_content2_untracked-content2)
>

Ah, great idea. Done.

PS: not sure what you use as a MUA but is appears to be quite bad at
> standard quoting.
>

Sorry, that's the new Google Inbox.
Matt Mackall - Nov. 3, 2014, 9:55 p.m.
On Sun, 2014-11-02 at 14:19 -0800, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1413682221 25200
> #      Sat Oct 18 18:30:21 2014 -0700
> # Branch stable
> # Node ID 9439327a222474b6af32ed9f717bfce81f90e53b
> # Parent  5df55ad6918a76870695dfe855168309bb5da27c
> test-revert: don't unnecessarily pipe through 'cat'

I fixed this and the false positive from check-code, thanks.

Patch

diff --git a/tests/test-revert.t b/tests/test-revert.t
--- a/tests/test-revert.t
+++ b/tests/test-revert.t
@@ -730,7 +730,7 @@ 
 
 Setup working directory
 
-  $ python ../gen-revert-cases.py wc | cat
+  $ python ../gen-revert-cases.py wc
   $ hg addremove --similarity 0
   removing added_removed
   removing added_revert