Re: misleading error message from variable modifier

[Chet Ramey]

On 3/18/18 1:01 AM, don fong wrote:
> will the coverage target be in an upcoming release?

Sure. It's in the devel branch now.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

will the coverage target be in an upcoming release?

On Sat, Mar 17, 2018 at 7:13 PM, Chet Ramey  wrote:

> On 3/17/18 6:39 PM, don fong wrote:
> > Chet, thanks for the tip about where to find the tests for subst.c .  i
> > still think that my tests cover some cases that aren't covered by
> > posixexp.tests .
>
> There are other test cases.
>
> > it's cool that you increased the coverage of subst.c .  how did you
> produce
> > the report?  i didn't see a script or makefile target to do it.
>
> I wrote a Makefile target to build a gcov-enabled binary and ran the
> test suite with it.
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/17/18 6:39 PM, don fong wrote:
> Chet, thanks for the tip about where to find the tests for subst.c .  i
> still think that my tests cover some cases that aren't covered by
> posixexp.tests .

There are other test cases.

> it's cool that you increased the coverage of subst.c .  how did you produce
> the report?  i didn't see a script or makefile target to do it.

I wrote a Makefile target to build a gcov-enabled binary and ran the
test suite with it.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

Chet, thanks for the tip about where to find the tests for subst.c .  i
still think that my tests cover some cases that aren't covered by
posixexp.tests .

it's cool that you increased the coverage of subst.c .  how did you produce
the report?  i didn't see a script or makefile target to do it.




On Wed, Mar 14, 2018 at 7:08 PM, Chet Ramey  wrote:

> On 3/9/18 3:14 AM, don fong wrote:
>
> >
> > my question was whether you have tests for the variable modifiers.
> > i don't see any.  that's the area of code i was touching, and that's why
> i
> > wrote a few tests of that area.
>
> Thank you for the inspiration. I ran the devel version of the test suite
> through gcov, added some tests, and was able to increase the coverage of
> subst.c (the word expansion code, plus) from 83% to 86%. It's tough to get
> it higher than that due to the functions that exist only for readline
> support and the debugging and system call error-handling code.
>
> Chet
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/9/18 3:14 AM, don fong wrote:

>  
> my question was whether you have tests for the variable modifiers.
> i don't see any.  that's the area of code i was touching, and that's why i
> wrote a few tests of that area.

Thank you for the inspiration. I ran the devel version of the test suite
through gcov, added some tests, and was able to increase the coverage of
subst.c (the word expansion code, plus) from 83% to 86%. It's tough to get
it higher than that due to the functions that exist only for readline
support and the debugging and system call error-handling code.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/9/18 3:14 AM, don fong wrote:
>  
> 
> If you'd like to augment the test suite where you feel it lacks something,
> please feel free to do so.
> 
> 
> tests were included in my patch.  you deleted them.  i think they should
> be added in.

I understand you're pretty chapped about that.  I'm surprised, but I guess
we all pick our hills to die on.

> 
> > are there any tests that cover the variable modifiers, either unit 
> tests or
> > functional tests?
> Yes. The test framework and tests are available for you to see.
> 
>  
> my question was whether you have tests for the variable modifiers.
> i don't see any.  that's the area of code i was touching, and that's why i
> wrote a few tests of that area.

You should probably look at some of the files in the `tests' subdirectory.
The `posixexp' series should have what you want, including the parameter
expansions you're asking about.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

> If you'd like to augment the test suite where you feel it lacks something,
> please feel free to do so.


tests were included in my patch.  you deleted them.  i think they should
be added in.

> are there any tests that cover the variable modifiers, either unit tests
> or
> > functional tests?
> Yes. The test framework and tests are available for you to see.


my question was whether you have tests for the variable modifiers.
i don't see any.  that's the area of code i was touching, and that's why i
wrote a few tests of that area.

On Thu, Mar 8, 2018 at 7:10 AM, Chet Ramey  wrote:

> On 3/7/18 1:00 PM, don fong wrote:
> > Chet Ramey wrote:
> >> What are the most important features that you consider to lack
> unit tests?
> >
> > are there any tests that cover the variable modifiers, either unit tests
> or
> > functional tests?
>
> Yes. The test framework and tests are available for you to see. They're all
> tests of shell behavior, since what concerns most people is shell behavior.
> If you'd like to augment the test suite where you feel it lacks something,
> please feel free to do so. I'm not interested in adding a new testing
> framework.
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/7/18 1:00 PM, don fong wrote:
> Chet Ramey wrote:
>> What are the most important features that you consider to lack unit tests?
> 
> are there any tests that cover the variable modifiers, either unit tests or
> functional tests?

Yes. The test framework and tests are available for you to see. They're all
tests of shell behavior, since what concerns most people is shell behavior.
If you'd like to augment the test suite where you feel it lacks something,
please feel free to do so. I'm not interested in adding a new testing
framework.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

Chet Ramey wrote:
> What are the most important features that you consider to lack unit tests?

are there any tests that cover the variable modifiers, either unit tests or
functional tests?

is any of subst.c covered by unit tests?

how do unit tests get run on this project?  i don't see a unit test target
in
the Makefile.  the "make tests" target mentioned in the INSTALL file,
seems to do only functional tests.




On Sun, Mar 4, 2018 at 1:59 PM, Chet Ramey  wrote:

> On 3/2/18 4:45 PM, don fong wrote:
> > Chet, thanks for the explanation about CHANGES.  i am not familiar with
> the
> > bash release process.
> >
> > as for this:
> >
> >> I didn't think the tests were necessary.
> >
> > what standard are you using to judge whether tests are necessary?  does
> the
> > bash project have any coverage metrics?
>
> It's admittedly subjective, but the bash test suite is very large and
> concentrates on testing code that has undergone more changes than this,
> and is more heavily used.
>
> In this case, I didn't think a fix that added two lines of code to a
> relatively stable function needed a test to ensure that it doesn't change.
>
> > as far as i can tell, the vast majority of the C code here seems to lack
> > unit tests as well.
>
> What are the most important features that you consider to lack unit tests?
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/2/18 4:45 PM, don fong wrote:
> Chet, thanks for the explanation about CHANGES.  i am not familiar with the
> bash release process.
> 
> as for this: 
> 
>> I didn't think the tests were necessary.
> 
> what standard are you using to judge whether tests are necessary?  does the
> bash project have any coverage metrics?

It's admittedly subjective, but the bash test suite is very large and
concentrates on testing code that has undergone more changes than this,
and is more heavily used.

In this case, I didn't think a fix that added two lines of code to a
relatively stable function needed a test to ensure that it doesn't change.

> as far as i can tell, the vast majority of the C code here seems to lack
> unit tests as well.

What are the most important features that you consider to lack unit tests?

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

Chet, thanks for the explanation about CHANGES.  i am not familiar with the
bash release process.

as for this:

> I didn't think the tests were necessary.

what standard are you using to judge whether tests are necessary?  does the
bash project have any coverage metrics?

on general principles, when someone makes a change, there ought to be a
test to show that it does what it's supposed to.  that's just good
engineering.  there also ought to be sufficient pre-existing tests to show
that the change didn't break anything else.  that goes double when patches
are being submitted by outsiders and newcomers (like me).

before i submitted my patch, i looked around in the test directory to see
if there were any tests that covered the behavior i was changing.  i didn't
see any tests of the variable modifiers at all.  that struck me as a
situation that should be changed.  so i added some tests to cover my own
change plus a bit more.

as far as i can tell, the vast majority of the C code here seems to lack
unit tests as well.

for a widely used program like bash, IMHO it'd be worthwhile to have
automated tests (either unit tests or functional tests) that cover every
documented feature.

if that hasn't been the case in the past, i propose that it be adopted as a
new goal.  there should at least be a rule that all new code should be
covered by tests.  i'm astonished to learn that the tests that i
contributed were simply discarded.






On Fri, Mar 2, 2018 at 12:21 PM, Chet Ramey  wrote:

> On 3/1/18 11:37 PM, don fong wrote:
> > Chet, thanks.  in subst.c there is code that looks similar to what i had
> > suggested.  but i don't see the tests that i submitted.  i also don't see
> > the change listed in CHANGES?
>
> I didn't think the tests were necessary. And the ChangeLog file is the
> one to look at (eventually resolves to CWRU/CWRU.chlog, as Clark noted).
> The CHANGES file is updated as part of release engineering; as you might
> suspect, I'm working on bash-5.0-alpha.
>
> Chet
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/1/18 11:37 PM, don fong wrote:
> Chet, thanks.  in subst.c there is code that looks similar to what i had
> suggested.  but i don't see the tests that i submitted.  i also don't see
> the change listed in CHANGES?

I didn't think the tests were necessary. And the ChangeLog file is the
one to look at (eventually resolves to CWRU/CWRU.chlog, as Clark noted).
The CHANGES file is updated as part of release engineering; as you might
suspect, I'm working on bash-5.0-alpha.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[Clark Wang]

On Fri, Mar 2, 2018 at 12:37 PM, don fong  wrote:

> Chet, thanks.  in subst.c there is code that looks similar to what i had
> suggested.  but i don't see the tests that i submitted.  i also don't see
> the change listed in CHANGES?
>

It's in the file CWRU/CWRU.chlog:

   2/24
   
subst.c
- parameter_brace_expand_error: add parameter saying whether or not
  we are checking whether value is null, so we can have different
  error messages for ${x:?} and ${x?}. Report and fix from
  don fong 

Re: misleading error message from variable modifier

[don fong]

Chet, thanks.  in subst.c there is code that looks similar to what i had
suggested.  but i don't see the tests that i submitted.  i also don't see
the change listed in CHANGES?







On Thu, Mar 1, 2018 at 11:09 AM, Chet Ramey  wrote:

> On 3/1/18 12:24 PM, don fong wrote:
> > any feedback on this patch?
>
> Check the current devel git branch.
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 3/1/18 12:24 PM, don fong wrote:
> any feedback on this patch?

Check the current devel git branch.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

any feedback on this patch?

On Sat, Feb 24, 2018 at 11:24 AM, Chet Ramey  wrote:

> On 2/23/18 10:20 PM, don fong wrote:
> > hi folks.  i'm a bash user, who just noticed a slight anomaly.  it has
> > to with the shell variable modifier ${parameter?} .  according to the
> > man page, ${X?} should yield an error message and exit if X is unset,
> > otherwise the value of X.  in this case,
> >
> > unset X; echo ${X?}
> >
> > i expect to get an error message, and indeed an error message results.
> >
> > bash: X: parameter null or not set
> >
> > but i think the error message is misleading.
>
> Thanks for the report and patch. I'll take a look.
>
> Chet
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
>

Re: misleading error message from variable modifier

[Chet Ramey]

On 2/23/18 10:20 PM, don fong wrote:
> hi folks.  i'm a bash user, who just noticed a slight anomaly.  it has
> to with the shell variable modifier ${parameter?} .  according to the
> man page, ${X?} should yield an error message and exit if X is unset,
> otherwise the value of X.  in this case,
> 
> unset X; echo ${X?}
> 
> i expect to get an error message, and indeed an error message results.
> 
> bash: X: parameter null or not set
> 
> but i think the error message is misleading.  

Thanks for the report and patch. I'll take a look.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/

Re: misleading error message from variable modifier

[don fong]

Clark, thanks for your response.  i'm attaching my patch.


On Fri, Feb 23, 2018 at 7:32 PM, Clark Wang  wrote:
> On Sat, Feb 24, 2018 at 11:20 AM, don fong  wrote:
>>
>>
>> i would like to submit this change for inclusion in bash.  how should i
>> proceed?
>
>
> It's very common to send patches directly to this mailing list. I believe
> it's also OK to send only to Chet. :)


myfix.patch
Description: Binary data

Re: misleading error message from variable modifier

[Clark Wang]

On Sat, Feb 24, 2018 at 11:20 AM, don fong  wrote:

>
> i would like to submit this change for inclusion in bash.  how should i
> proceed?
>

It's very common to send patches directly to this mailing list. I believe
it's also OK to send only to Chet. :)

misleading error message from variable modifier

[don fong]

hi folks.  i'm a bash user, who just noticed a slight anomaly.  it has
to with the shell variable modifier ${parameter?} .  according to the
man page, ${X?} should yield an error message and exit if X is unset,
otherwise the value of X.  in this case,

unset X; echo ${X?}

i expect to get an error message, and indeed an error message results.

bash: X: parameter null or not set

but i think the error message is misleading.  the wording sort of
implies that null would be an error, even though with the ? modifier,
null is OK.  it's kind of confusing as well, to see the same error
message from both ${X?} and ${X:?} when they are really checking
distinct things.

i think the error message should say

bash: X: parameter is not set

a co-worker challenged me to submit a patch.  so i downloaded the
code, looked at it, made a fix, and created a test script compatible
with the existing tests/ framework.  all tests pass.  (i glanced
around in the tests/ directory, did not see any tests of the variable
modifiers, so i extended my tests to cover some of the other modifiers
as well.)

i would like to submit this change for inclusion in bash.  how should i proceed?