don't record empty IFS scan regions

Commit Message

Martijn Dekker March 22, 2018, 2:40 a.m. UTC

evalvar() records empty expansion results (varlen == 0) as string
regions that need to be scanned for IFS characters. This is pointless,
because there is nothing to split.

This patch fixes the bug that, given no positional parameters, unquoted
$@ and $* incorrectly generate one empty field (they should generate no
fields). Apparently that was a side effect of the above.

- M.

Comments

Herbert Xu March 22, 2018, 9:35 a.m. UTC | #1

Martijn Dekker <[e-mail]> wrote:
> 
> evalvar() records empty expansion results (varlen == 0) as string
> regions that need to be scanned for IFS characters. This is pointless,
> because there is nothing to split.
> 
> This patch fixes the bug that, given no positional parameters, unquoted
> $@ and $* incorrectly generate one empty field (they should generate no
> fields). Apparently that was a side effect of the above.

Please attach a reproducer as I cannot reproduce this problem.

Thanks,

Martijn Dekker March 22, 2018, 10 a.m. UTC | #2

Op 22-03-18 om 10:35 schreef Herbert Xu:
> Martijn Dekker <[e-mail]> wrote:
>>
>> evalvar() records empty expansion results (varlen == 0) as string
>> regions that need to be scanned for IFS characters. This is pointless,
>> because there is nothing to split.
>>
>> This patch fixes the bug that, given no positional parameters, unquoted
>> $@ and $* incorrectly generate one empty field (they should generate no
>> fields). Apparently that was a side effect of the above.
> 
> Please attach a reproducer as I cannot reproduce this problem.

Sorry, I forgot to mention it only occurs with set & empty IFS.

It came up earlier in the thread about Harald van Dijk's patch for a
recursive parser.

$ dash-0.5.9.1 -c 'IFS=; set --; set -- $@ $*; echo $#'
2

(expected output: 0)

I'll resend the patch with this reproducer.

- M.
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Harald van Dijk March 22, 2018, 7:28 p.m. UTC | #3

On 22/03/2018 03:40, Martijn Dekker wrote:
> evalvar() records empty expansion results (varlen == 0) as string
> regions that need to be scanned for IFS characters. This is pointless,
> because there is nothing to split.

varlen may be set to -1 when an unset variable is expanded. If it's 
beneficial to avoid recording empty regions to scan for IFS characters, 
should those also be excluded?

> This patch fixes the bug that, given no positional parameters, unquoted
> $@ and $* incorrectly generate one empty field (they should generate no
> fields). Apparently that was a side effect of the above.

This seems weird though. If you want to remove the recording of empty 
regions because they are pointless, then how does removing them fix a 
bug? Doesn't this show that empty regions do have an effect? Perhaps 
they're not supposed to have any effect, perhaps it's a specific 
combination of empty regions and something else that triggers some bug, 
and perhaps that combination can no longer occur with your patch.

Cheers,
Harald van Dijk
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/src/expand.c b/src/expand.c
index 705fef7..03a9b0c 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -771,7 +771,7 @@  vsplus:
 
 	if (subtype == VSNORMAL) {
 record:
-		if (!easy)
+		if (!easy || varlen == 0)
 			goto end;
 		recordregion(startloc, expdest - (char *)stackblock(), quoted);
 		goto end;