[Edbrowse-dev] wordexp again

[Edbrowse-dev] wordexp again

[Chris Brannon]

Found a bug in nextScanFile, from stringfile.c.  To reproduce from the
shell:

mkdir 'foo bar'
touch 'foo bar'/{1,2,3} # Create 3 files
edbrowse 'foo bar'
You'll get a buffer full of junk, as the code is reading past the end of
an array.

There also seems to be a regression with tilde expansion.
>From edbrowse, e ~ used to visit the home directory.  As of the latest,
I need to use e ~/.  No big deal; it's one extra character ... but it is
a regression.

-- Chris

[Edbrowse-dev] wordexp again

[Karl Dahlke]

I fixed the ~ alone bug, it was just one line of code.

As for your other bug foobar/{1,2,3}
I can't reproduce it. It works fine for me.
I edit foobar and get the 3 files.
Not sure how to procede with this one.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Chris Brannon]

Karl Dahlke <eklhad@comcast.net> writes:

> As for your other bug foobar/{1,2,3}
> I can't reproduce it. It works fine for me.

Did you miss the space in the directory name?

-- Chris

[Edbrowse-dev] wordexp again

[Karl Dahlke]

Ok, it's fixed.
Forgot that space is a shell meta character as far as wordexp is concerned.
Also tab.

Karl Dahlke

[Edbrowse-dev] wordexp again

[Karl Dahlke]

I made one common routine to protect a file name or directory name from
shell expansion, it's the same code I was using before, but now it is shared
between the directory scan and the plugin commands.
Ok, that's good software practice, but it exposed a bug
that I think was there all along.
You can't edit a directory with a backslash in it.

mkdir "a\\b"
cd a?b
touch foo bar
cd ..
edbrowse a?b

Try it before or after my latest push.
The files therein just don't show up,
because wordexp doesn't expand a\\b/* properly.
and that is the proper escaping.
At the shell prompt
ls a\\b/*
works fine.
And it all works through the plugin system, i.e. a\b.pdf becomes
pdftohtml a\\b.pdf
This seems to be a bug in wordexp, at least my wordexp,
and I'm really not sure what to do about it.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Thu, Apr 16, 2015 at 05:40:53PM -0400, Karl Dahlke wrote:
> I made one common routine to protect a file name or directory name from
> shell expansion, it's the same code I was using before, but now it is shared
> between the directory scan and the plugin commands.
> Ok, that's good software practice, but it exposed a bug
> that I think was there all along.
> You can't edit a directory with a backslash in it.
> 
> mkdir "a\\b"
> cd a?b
> touch foo bar
> cd ..
> edbrowse a?b
> 
> Try it before or after my latest push.
> The files therein just don't show up,
> because wordexp doesn't expand a\\b/* properly.
> and that is the proper escaping.
> At the shell prompt
> ls a\\b/*
> works fine.
> And it all works through the plugin system, i.e. a\b.pdf becomes
> pdftohtml a\\b.pdf
> This seems to be a bug in wordexp, at least my wordexp,
> and I'm really not sure what to do about it.

I can reproduce it, but I wasn't aware we were passing these to wordexp.
I'll look into this if you want.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

On Fri, Apr 17, 2015 at 08:12:43AM +0100, Adam Thompson wrote:
> On Thu, Apr 16, 2015 at 05:40:53PM -0400, Karl Dahlke wrote:
> > I made one common routine to protect a file name or directory name from
> > shell expansion, it's the same code I was using before, but now it is shared
> > between the directory scan and the plugin commands.
> > Ok, that's good software practice, but it exposed a bug
> > that I think was there all along.
> > You can't edit a directory with a backslash in it.
> > 
> > mkdir "a\\b"
> > cd a?b
> > touch foo bar
> > cd ..
> > edbrowse a?b
> > 
> > Try it before or after my latest push.
> > The files therein just don't show up,
> > because wordexp doesn't expand a\\b/* properly.
> > and that is the proper escaping.
> > At the shell prompt
> > ls a\\b/*
> > works fine.
> > And it all works through the plugin system, i.e. a\b.pdf becomes
> > pdftohtml a\\b.pdf
> > This seems to be a bug in wordexp, at least my wordexp,
> > and I'm really not sure what to do about it.

Actually we broke things when doing wordexp implementation.
Basically the "quick check" in envFile is just wrong and needs to go since it
doesn't work correctly and doesn't include all corner cases.
Whilst looking at this I also noticed that ~username/whatever was also not
expanded due to this "quick check". Removing the check fixes this bug and
probably makes the behaviour much more consistant into the bargain (i.e.
` suppresses it, otherwise we get wordexp expansion).
Once I get the readline completion using the shellProtect routine I'll push the fix.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> Basically the "quick check" in envFile is just wrong and needs to go since it

And yet, I feel like this check was doing something from the user's perspective.
Like maybe edbrowse only tries to expand a filename through the shell
if you really intended that it do that.
So it wouldn't try to expand just because the file had a parenthesis or some such.
Maybe that doesn't matter IDK.
And is this really involved at all in the directory backslash bug that started this thread?

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Fri, Apr 17, 2015 at 09:33:11AM -0400, Karl Dahlke wrote:
> > Basically the "quick check" in envFile is just wrong and needs to go since it
> 
> And yet, I feel like this check was doing something from the user's perspective.
> Like maybe edbrowse only tries to expand a filename through the shell
> if you really intended that it do that.
> So it wouldn't try to expand just because the file had a parenthesis or some such.
> Maybe that doesn't matter IDK.
> And is this really involved at all in the directory backslash bug that started this thread?

Yep, removed it and the bug went away.
I've pushed the fix as is since I think that most of the time it'll do what we expect.
The way I look at it, if you use the command line with file names with special
chars in them you probably have to escape them anyway.
This way we get a consistant behaviour rather than broken shell expansions and 
other oddness.

I still need to do the readline tab completion fix, that's a little more difficult.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

>  quick check, and when to expand the line

Yes I remember now - if you don't do the quick check
and you want to edit any file with any such character,
having a quote or apostrophe or parenthesis or some such,
like
e foo'bar
it will fail if it goes through wordexp.
You'd have to write
e foo\'bar
and who wants to do that?
What casual user is going to remember to do that or understand?
The error will really confuse people.
I was trying to keep the user interface
simple and clean for the masses, yet powerful for those of us
who understand and want to take advantage of the
mysteries of shell expansion.

Karl Dahlke

[Edbrowse-dev] wordexp again

[Karl Dahlke]

I pulled the latest, withthe quick check removed,
and I don't like it, or I'm just not use to it.
This is a change in the user interface,
and if we all agree it should be made, then I also need to change the documentation.
I designed it so that it would not be hard or complicated
or confusing to edit files, like a'b or a(b or a|b or a[b,
and now these all produce errors, where they didn't before.
Errors because they all run through wordexp, and they didn't before.

e a(b
unexpected error while evaluating the shell pattern

What is the user suppose to do with that??

edbrowse use to run filenames through wordexp when
it is "reasonable" to do so, as per my check, and as per the following section
from my documentation.

------------------------------------------------------------

Whenever a file is read from or written to disk, $var, in the filename,
is replaced with the corresponding environment variable.
Thus you can edit your address book at any time via `e $adbook',
provided $adbook has been set in your environment.
Also, a leading ~/ is replaced with $HOME/,
making it easy to edit files in your home directory such as ~/.profile.

Shell meta characters are also expanded, provided the result is one file name.
You can read or write a file by typing a minimal portion of its name.
e work/foo* to get work/foobar.

Expansion is performed by wordexp(3), simulating a posix shell.
Unfortunately this has certain side effects.
Meta characters such as | & ' < ; etc will derail the expansion,
unless escaped by a backslash. This is confusing at first,
so think of it this way: "If I want to expand a file name using variables or
wild cards, i.e. a string containing $ [] * ?
or a leading ~/, I have to type it in as though I were typing at the shell."
An example might be $phones/at\&t. Even spaces must be escaped with backslash
or put in quotes. This complexity only occurs
if you are accessing environment variables or globbing.

------------------------------------------------------------

So the quick check was there for a reason,
and I don't think it was particularly buggy.
I don't want us to undo each other's work, that isn't productive,
so we should decide if we want this feature, yes or no,
and if we do put it back in,
and if we don't then I need to change the documentation.
I like the feature, because I don't want to have to escape things with backslashes all the time,
except on that rare occasion that I am invoking wordexp and need to escape things for  wordexp.
Another option is to forget wordexp and go back to the code
that I wrote 5 years ago, my own variable expansion and globbing,
which worked, and didn't have any of this '"()|; confusion around it.
It did what you want and didn't have any side effects.
Maybe I should have left well enough alone.
But we are where we are, and from here I guess I'd like to put my quick check
back in, perhaps modified if you think it has some bugs.

By the way, removing quick check had nothing to do with the bug
that I posted at the top of this thread.
You stil can't edit a directory with backslash in the name,
and I think that is a bug in wordexp that we can't get around.
That's maybe another reason to go back to my home grown code.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 4705 bytes --]

On Fri, Apr 17, 2015 at 02:01:13PM -0400, Karl Dahlke wrote:
> I pulled the latest, withthe quick check removed,
> and I don't like it, or I'm just not use to it.
> This is a change in the user interface,
> and if we all agree it should be made, then I also need to change the documentation.
> I designed it so that it would not be hard or complicated
> or confusing to edit files, like a'b or a(b or a|b or a[b,
> and now these all produce errors, where they didn't before.
> Errors because they all run through wordexp, and they didn't before.
> 
> e a(b
> unexpected error while evaluating the shell pattern
> 
> What is the user suppose to do with that??
> 
> edbrowse use to run filenames through wordexp when
> it is "reasonable" to do so, as per my check, and as per the following section
> from my documentation.
> 
> ------------------------------------------------------------
> 
> Whenever a file is read from or written to disk, $var, in the filename,
> is replaced with the corresponding environment variable.
> Thus you can edit your address book at any time via `e $adbook',
> provided $adbook has been set in your environment.
> Also, a leading ~/ is replaced with $HOME/,
> making it easy to edit files in your home directory such as ~/.profile.
> 
> Shell meta characters are also expanded, provided the result is one file name.
> You can read or write a file by typing a minimal portion of its name.
> e work/foo* to get work/foobar.
> 
> Expansion is performed by wordexp(3), simulating a posix shell.
> Unfortunately this has certain side effects.
> Meta characters such as | & ' < ; etc will derail the expansion,
> unless escaped by a backslash. This is confusing at first,
> so think of it this way: "If I want to expand a file name using variables or
> wild cards, i.e. a string containing $ [] * ?
> or a leading ~/, I have to type it in as though I were typing at the shell."
> An example might be $phones/at\&t. Even spaces must be escaped with backslash
> or put in quotes. This complexity only occurs
> if you are accessing environment variables or globbing.
> 
> ------------------------------------------------------------
> 
> So the quick check was there for a reason,
> and I don't think it was particularly buggy.
> I don't want us to undo each other's work, that isn't productive,
> so we should decide if we want this feature, yes or no,
> and if we do put it back in,
> and if we don't then I need to change the documentation.
> I like the feature, because I don't want to have to escape things with backslashes all the time,
> except on that rare occasion that I am invoking wordexp and need to escape things for  wordexp.
> Another option is to forget wordexp and go back to the code
> that I wrote 5 years ago, my own variable expansion and globbing,
> which worked, and didn't have any of this '"()|; confusion around it.
> It did what you want and didn't have any side effects.
> Maybe I should have left well enough alone.
> But we are where we are, and from here I guess I'd like to put my quick check
> back in, perhaps modified if you think it has some bugs.
> 
> By the way, removing quick check had nothing to do with the bug
> that I posted at the top of this thread.
> You stil can't edit a directory with backslash in the name,
> and I think that is a bug in wordexp that we can't get around.
> That's maybe another reason to go back to my home grown code.
> 
> Karl Dahlke
> _______________________________________________
> Edbrowse-dev mailing list
> Edbrowse-dev@lists.the-brannons.com

It fixed my test case when I tested it,
which appears to be *another* bug which looked similar to this one.
With it as it stands now, typing:
e a\\b
When you've got a directory named 'a\b' in the current directory allows you to
edit the correct thing (i.e. typing 'f' yields 'a\b')
but the files aren't visible in this case.
This isn't an escaping bug in wordexp as otherwise the double '\' would still be there.
I suspect we're double expanding or something somewhere, I need to check.
As for the quick check, there are corner cases it misses,
like the ~username syntax which is surprisingly useful once you get used to it.
I think the better way round this would be to reverse the leading `
since readline completion doesn't do wordexp-safe file names at the moment (and
making it do that is difficult). This would work for me as a user since it'd
mean "treat this filename specially".
That'd get rid of all the random confusion and need to escape things,
apart from in the very rare cases that you genuinely want an ` at the start of
your file name.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On Fri, Apr 17, 2015 at 10:34:05PM +0100, Adam Thompson wrote:
> > By the way, removing quick check had nothing to do with the bug
> > that I posted at the top of this thread.
> > You stil can't edit a directory with backslash in the name,
> > and I think that is a bug in wordexp that we can't get around.
> > That's maybe another reason to go back to my home grown code.

No, it's a bug in our code. I've checked and we've got a couple of interesting
things happening. With wordexp enabled, we're duble expanding the filename,
once before we pass it to sortedDirList, and once in nextScanFile.
Without wordexp expansion, we don't expand the file name before we get to
nextScanFile, but then the single \ is swallowed by wordexp so the glob is incorrect.
I should've spotted this when we first switched to using wordexp because the
call in nextScanFile really should be to glob rather than wordexp.
Better still, I think we just shouldn't have switched nextScanFile to wordexp
since we just want a directory listing in this case.

I've also tried out the inverted ` character and that seems to make much more
sense in terms of usable behaviour.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

Reversing the sense of the leading ` is, I think, an interesting idea.
There are no side effects unless you ask for them, and it would fix a bug
that you introduced, wherein r5,
to read the fifth buffer into the current buffer, no longer works.
It doesn't work because 5 runs through wordexp and there is no file called 5.
I use this feature a hundred times a day, to cut and paste text
between sessions.
Anyways it fails now because 5 runs through wordexp,
but it use to work,
and would work again if such lines did not go through wordexp by default,
either because my syntax checker was back in place,
or because you had to indicate so with a leading `.
An unknown for me is how often I automatically, and without thinking,
use * to edit or read a file; I just pattern match automatically,
and I can see myself cursing and having to type it in again with a leading `
and by that time I probably could have just typed in the filename. Damn!
and I'd be annoyed because here again it is different from the shell.
I guess I'll have to sleep on this for a night or two,
or maybe play with it in its different modes for a week or two,
to get a sense of things.
One thing I have to do right away is fix the r5 bug,
in one way or another.
Reading and writing among buffers just has to work for me.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

On Fri, Apr 17, 2015 at 06:25:31PM -0400, Karl Dahlke wrote:
> Reversing the sense of the leading ` is, I think, an interesting idea.
> There are no side effects unless you ask for them, and it would fix a bug
> that you introduced, wherein r5,
> to read the fifth buffer into the current buffer, no longer works.
> It doesn't work because 5 runs through wordexp and there is no file called 5.
> I use this feature a hundred times a day, to cut and paste text
> between sessions.

Oops, yeah, that's an unfortunate one.
I've pushed the change since I had the patch handy.
Once we get used to it I think it makes sense as a change since ` now means
shell expand with all that that implies.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

Ok the first thing I notice with ` reversed is that r5 works again,
and that makes me happy.
I then notice that I still get 0 when I edit the directory a\b
and I will look into that one some more.
A simple debug print shows the string passed to wordexp is
a\\b/*
which is correct.
Doesn't matter how we got here, it is correct, and should expand properly,
but it does not,
so I still say, as I did from the start,
there is a bug in wordexp.
Maybe it's not important, not a high runner case for us,
or maybe I can find a way around it,
or maybe we need to use glob() for directory scan,
though that would not solve the case of
e `a\b/foo*
IDK.
The third thing I notice is that cd ~user/work doesn't expand
any more, unless I use `, and I think in this case,
the cd command, it's understood to be a shell thing,
so I propose changing this one to expand by default.
I can make that change, unless people disagree.


Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

On Fri, Apr 17, 2015 at 10:58:26PM +0100, Adam Thompson wrote:
> On Fri, Apr 17, 2015 at 10:34:05PM +0100, Adam Thompson wrote:
> > > By the way, removing quick check had nothing to do with the bug
> > > that I posted at the top of this thread.
> > > You stil can't edit a directory with backslash in the name,
> > > and I think that is a bug in wordexp that we can't get around.
> > > That's maybe another reason to go back to my home grown code.
> 
> No, it's a bug in our code. I've checked and we've got a couple of interesting
> things happening. With wordexp enabled, we're duble expanding the filename,
> once before we pass it to sortedDirList, and once in nextScanFile.
> Without wordexp expansion, we don't expand the file name before we get to
> nextScanFile, but then the single \ is swallowed by wordexp so the glob is incorrect.
> I should've spotted this when we first switched to using wordexp because the
> call in nextScanFile really should be to glob rather than wordexp.
> Better still, I think we just shouldn't have switched nextScanFile to wordexp
> since we just want a directory listing in this case.

In the case of sortedDirList we should be using scandir with the alphasort
function as a comparison function (then loading into your line map probably),
for the NextScanFile function (used in fetchmail.c I think)
we probably want to go back to readdir.
I'm happy to put together a fix for this tomorrow.

> I've also tried out the inverted ` character and that seems to make much more
> sense in terms of usable behaviour.

Any thoughts? I think this makes more sense to me since I don't have to be
trying to work out what and when I need to escape things, i.e.
with the "quick check" if I use ~/ then I have to shell escape everything,
or if I use $ then I have to escape everything.
Whereas with the ` character acting to *enable* expansion,
then if I specify ` at the start of the filename then I know I need to shell
escape everything because I'm getting wordexp.
The change I made also had a \ escape for ` in the rare case that the leading ` is required.
If you're happy with this then I'll test and push this change.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> we probably want to go back to readdir.

I would rather not do this.
It is not portable, for one thing.
And the wordexp * protected code will expand directories
just fine, if not double expanded, and it would not be double expanded
if you had your reversed ` convention in place.

Not trying to have all of us move all the chess pieces at once, so
why don't you push your new ` convention first,
and we'll all play with it for a bit,
and see that directory scans work just fine, and then take the next step
whatever that may be.
For files with leading ` I would use the convention 
``filename
just like you use %% for % in printf.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Fri, Apr 17, 2015 at 06:39:07PM -0400, Karl Dahlke wrote:
> > we probably want to go back to readdir.
> 
> I would rather not do this.
> It is not portable, for one thing.
It's as portable as wordexp. Both of them are posix.
Probably neither of them are DOS. Also if we go for scandir, like glob,
we can get the sorting we want via the alphasort comparisn function.

> Not trying to have all of us move all the chess pieces at once, so
> why don't you push your new ` convention first,
> and we'll all play with it for a bit,
> and see that directory scans work just fine, and then take the next step
> whatever that may be.
> For files with leading ` I would use the convention 
> ``filename
> just like you use %% for % in printf.

I've gone for \` since that's shell escaping which makes more sense in this case I think, though I don't really care.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

Well a very smart person told me to write a 10 line C program. Here it is.

#include <stdio.h>
#include <wordexp.h>
main(int argc, char **argv)
{
        wordexp_t w;
char line[80];
while(gets(line)) {
wordexp(line, &w, 0);
printf("%s\n", w.we_wordv[0]);
        wordfree(&w);
}
}

Create the two files abc and ab\d
Run the program and type in various strings.
Here is input and output.

jkl jkl
\\ \
\\\\ \\
ab? abc
ab\\e ab\e
ab\\* ab\*  (should be ab\d)
ab\\$HOME ab\/home/eklhad
$HOME\\ab /home/eklhad/\ab
\\'j' \j
ab\\\\* ab\d

So backslashes are crunched once, and \' becomes ', and \| becomes |,
and so on as $variables are replaced.
Then, the next pass, globbing, if globbing occurs,
or if you want it to occur, then \\ becomes \ once again.
However \' does not become ', only \\ is crunched.
This second crunching is the bug, and there is really no way around it.
That's why ab\\\\* expands the way ab\\* should.

If I have the energy, and I'm not sure if I do,
but if I do, I believe the right answer is to write my own wordexp function
that does the following:

pass 1
ignore ' " | () [] ; \ blah blah blah,
no nasty side effects of calling this function.
No confusion, and no reason not to run it all the time.
don't have to start with a ` to invoke it, just run it because
it doesn't do anything weird.
Replace $var with its environment value,
and maybe even ${var}.
Let's be honest, this software is easy, and entirely portable.

pass 2
Call glob() to expand any shell wild cards.
This is the hard part, so let the library do that,
but glob doesn't screw up other characters in the string.

That's what I will sleep on tonight,
and see how I feel about it in the morning.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 4031 bytes --]

On Sat, Apr 18, 2015 at 12:18:00AM -0400, Karl Dahlke wrote:
> Well a very smart person told me to write a 10 line C program. Here it is.
> 
> #include <stdio.h>
> #include <wordexp.h>
> main(int argc, char **argv)
> {
>         wordexp_t w;
> char line[80];
> while(gets(line)) {
> wordexp(line, &w, 0);
> printf("%s\n", w.we_wordv[0]);
>         wordfree(&w);
> }
> }
> 
> Create the two files abc and ab\d
> Run the program and type in various strings.
> Here is input and output.
> 
> jkl jkl
> \\ \
> \\\\ \\
> ab? abc
> ab\\e ab\e
> ab\\* ab\*  (should be ab\d)
> ab\\$HOME ab\/home/eklhad
> $HOME\\ab /home/eklhad/\ab
> \\'j' \j
> ab\\\\* ab\d
> 
> So backslashes are crunched once, and \' becomes ', and \| becomes |,
> and so on as $variables are replaced.
> Then, the next pass, globbing, if globbing occurs,
> or if you want it to occur, then \\ becomes \ once again.
> However \' does not become ', only \\ is crunched.
> This second crunching is the bug, and there is really no way around it.
> That's why ab\\\\* expands the way ab\\* should.
> 
> If I have the energy, and I'm not sure if I do,
> but if I do, I believe the right answer is to write my own wordexp function
> that does the following:
> 
> pass 1
> ignore ' " | () [] ; \ blah blah blah,
> no nasty side effects of calling this function.
> No confusion, and no reason not to run it all the time.
> don't have to start with a ` to invoke it, just run it because
> it doesn't do anything weird.
> Replace $var with its environment value,
> and maybe even ${var}.
> Let's be honest, this software is easy, and entirely portable.
> 
> pass 2
> Call glob() to expand any shell wild cards.
> This is the hard part, so let the library do that,
> but glob doesn't screw up other characters in the string.
> 
> That's what I will sleep on tonight,
> and see how I feel about it in the morning.

I notice you miss out the ~whatever expansion here,
which is a missing feature of this. As I said,
I think the ` inverting probably fixes most of this,
with the caveat about the cd command.
I also think we shouldn't be using glob to scan a directory,
we should be using scandir and alphasort,
no need to append / or /* to strings in this case and no other oddness possibly
caused by odd directory names (I've got a suspician there's another escaping
bug with this code somewhere).
I'm also not entirely sure I'd mind having command substitution enabled,
particularly as I use this a lot in file name expansions now.
I definitely think we need to talk about this,
particularly now we have a more or less fixed version of edbrowse.
What the last couple of days have shown is that making undiscussed changes is
just going to get circular due to differing user requirements and opinions.
In the environment inwhich I use Edbrowse,
the more file name expansion features I can have enabled the better,
however other people aren't as comfrtable in the shell.
I'd quite like a sort of hybrid approach actually which keeps the ` character
for full wordexp (with command substitution as well)
but may be some sort of glob (plus ~ expansion) without it.
If you're specifying environment variables then your probably in the realm of
the shell, and then you may as well have all the power that implies.

@Karl: I think one of the other issues here is that you've been using the code
the way you wrote it for years, whereas I'm coming to the env var expansion
stuff fairly fresh and based on years of using the shell.
That gives two different perspectives on what the "right"
thing is in this case.
As for the bug in wordexp, I need to read through the posix standard to se how it
handles escaping to see if this is a genuine bug or just us not understanding
how wordexp is supposed to work with quoting.
At the end of the day, people claim wordexp's expansion is the same as the
shell, but it's not the same function and thus the expansions are slightly different.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> What the last couple of days have shown is that making undiscussed changes is
> just going to get circular due to differing user requirements and opinions.

Yes and we all did that, when initially my intent was just to fix a bug.
But instead we changed the user interface, things expanded when they didn't before,
and conversely, changing the meaning of ` etc,
and some of the edbrowse users, who are not on this list
contacted me and were confused.
So now it works all as it did before, except the bug is fixed,
and the code is much cleaner inside.
Of course there's nothing wrong with cleaning up code,
but we do have to talk more and plan more
before changing the user interface.

I had the same problem with some of the plugin work,
which we all agreed was great, and even the users agreed was great, so no trouble there,
they are really quite enthusiastic about it,
but we rather innocently said,

Hey let's change mime { to plugin { in .ebrc

Sure that makes sense, but some of my users out there had edbrowse fail on startup
for reasons they didn't understand.
And even if they waited til the next release, it would have been the same problem.
No backward compatibility.
So at their suggestion I allow either syntax now
mime { or plugin {
and edbrowse runs again with the previous .ebrc

I have to temper what I do, and what we do, with some feedback that I get
from others that is, unfortunately, not always on this list.
It is, I suppose, my job to relay some of this information to this list,
so we can all talk about it.
Sorry if I missed some of these steps.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

On Sat, Apr 18, 2015 at 07:34:35AM -0400, Karl Dahlke wrote:
> > What the last couple of days have shown is that making undiscussed changes is
> > just going to get circular due to differing user requirements and opinions.
> 
> Yes and we all did that, when initially my intent was just to fix a bug.
> But instead we changed the user interface, things expanded when they didn't before,
> and conversely, changing the meaning of ` etc,
> and some of the edbrowse users, who are not on this list
> contacted me and were confused.
> So now it works all as it did before, except the bug is fixed,
> and the code is much cleaner inside.
> Of course there's nothing wrong with cleaning up code,
> but we do have to talk more and plan more
> before changing the user interface.

I think this also comes back to release management as well, i.e.
users really shouldn't be pulling the latest git and expecting everything to be
fully working, that's not what version control's for in a collaberative project.
If people are running the latest, bleeding edge,
code then things will break like this.
Obviously I hope some users do keep doing this since user feedback is always
welcome, but we're a development team now and we need to be kept in the loop if
this kind of thing is happening.

One thing that's on my mind at the moment is that,
when I seriously get stuck into the DOM stuff,
I'll need people to work with me on that and I'd rather not do it with a gun to my head i.e.
users pulling git changes and expecting the same browsing experience etc.
That just won't happen due to the amount of changes.
I appreciate not everyone's on this list,
but I'd personally change the github page to point to this list rather than the
commandline list since git is our development environment.
We post released code (at least I hope we do) and binaries,
if users want a stable browser they should probably use that.
Of course this also means we need to be better at getting features out,
thus preventing users from having to use our git repo to get anything close to
the latest code.
I'd like to head towards more of a monthly or 2 monthly release cycle if we
can, with small point releases for features.
For example the plugin changes warrent a point release.
Obviously if there're no new features then there's no new release but we should
be aiming to get features out as soon as they're stable really.
That'll also have the nice side effect of keeping the project moving,
which is always a good thing.
That being said, I accept that the build process is rather involved at the
minute, so we may need to work to automate some of that.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Edbrowse-dev] wordexp again

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> users really shouldn't be pulling the latest git and expecting everything to be
> fully working, that's not what version control's for in a collaberative project.

Yes.  Basically, if you're going to run from git, you need to follow
this list.  Also note if you have an NNTP client, you can even follow
it without subscribing.  Just point at news.gmane.org and read the
gmane.comp.web.edbrowse.devel newsgroup.  I should probably add a note
about that to the website and README.  There are lots of people out
there who prefer to remain anonymous, and they'd benefit from that interface.

> but I'd personally change the github page to point to this list rather than the
> commandline list since git is our development environment.

Yes, but the folks Karl is discussing are contacting him directly.
I follow both lists, and the other has been silent for a while.

> That being said, I accept that the build process is rather involved at the
> minute, so we may need to work to automate some of that.

The only thing that is fiddly is the binary build process.
Releasing the code is easy.

Your message has reminded me of another problem, though.
Right now, the edbrowse web site is hosted on my server.  If it needs to
change, for whatever reason, I'm the only one who can change it.
Currently, I haven't come up with a fix that makes me completely happy,
but I'll keep thinking on it.
You guys may end up with some kind of (restricted) ssh access to the
machine hosting the site.  I could also set up a page on github, say
edbrowse.github.com, and we could all push to that.  But github doesn't
make it easy to host binary content like tarballs and executables, I think.

-- Chris

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

On Sat, Apr 18, 2015 at 12:33:48PM -0700, Chris Brannon wrote:
> Adam Thompson <arthompson1990@gmail.com> writes:
> 
> > users really shouldn't be pulling the latest git and expecting everything to be
> > fully working, that's not what version control's for in a collaberative project.
> 
> Yes.  Basically, if you're going to run from git, you need to follow
> this list.  Also note if you have an NNTP client, you can even follow
> it without subscribing.  Just point at news.gmane.org and read the
> gmane.comp.web.edbrowse.devel newsgroup.  I should probably add a note
> about that to the website and README.  There are lots of people out
> there who prefer to remain anonymous, and they'd benefit from that interface.

Fair enough, and that's a cool interface I was unaware of.

> > but I'd personally change the github page to point to this list rather than the
> > commandline list since git is our development environment.
> 
> Yes, but the folks Karl is discussing are contacting him directly.
> I follow both lists, and the other has been silent for a while.

True, but I remember that we were getting github issues there (I think).

> > That being said, I accept that the build process is rather involved at the
> > minute, so we may need to work to automate some of that.
> 
> The only thing that is fiddly is the binary build process.
> Releasing the code is easy.
> 
> Your message has reminded me of another problem, though.
> Right now, the edbrowse web site is hosted on my server.  If it needs to
> change, for whatever reason, I'm the only one who can change it.
> Currently, I haven't come up with a fix that makes me completely happy,
> but I'll keep thinking on it.
> You guys may end up with some kind of (restricted) ssh access to the
> machine hosting the site.  I could also set up a page on github, say
> edbrowse.github.com, and we could all push to that.  But github doesn't
> make it easy to host binary content like tarballs and executables, I think.

Hmmm, what about sftp only ssh accounts chrooted to the web dir? Would that work?
Or an ftp server installation, again with virtual user accounts (or local
accounts with disabled logins)? That could even use ftps.
I've done such an installation before and it works fairly well.
The main problem with these is versioning, not of binaries,
they will be versioned in the name, but for web content.
For this we could probably use a git repo which we could all push to.
The server could then pull from this repo and apply the updates,
with binaries being uploaded (by you currently,
but via the ftp or sftp mechanisms mentioned above in future) separately.
I don't want to host the edbrowse website on github,
that's not a web hosting platform. If you're happy hosting it then that's
brilliant, otherwise we can probably sort out alternative arrangements like a
vm running somewhere which we all have access to.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Edbrowse-dev] wordexp again

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> Hmmm, what about sftp only ssh accounts chrooted to the web dir? Would that work?

I need a public key from each of you.
Yes, this sounds really good!  sftp for you guys, chrooted to the
edbrowse web directory.  All the textual content goes into a git
repo on the VM where the site is hosted.  The web server stays in sync
with that repo, and we all have push access.
I need to get cracking and figure out how to use gitolite.
It may also be time to move edbrowse off to its own subdomain,
say, edbrowse.the-brannons.com.

> I don't want to host the edbrowse website on github,
> that's not a web hosting platform.

Well that isn't its primary intent, but with Github Pages, you can turn
it into a web hosting platform, if all your content is static text.
Lots of people are hosting blogs on it these days, for better or worse.
I have my own complaints about GitHub, though.

-- Chris

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> I notice you miss out the ~whatever expansion here,

No, I get that for free.
It's handled by glob().
And it works, and it's pretty cool.
I don't have a lot of use for it but I like it.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

On Sat, Apr 18, 2015 at 08:36:45AM -0400, Karl Dahlke wrote:
> > I notice you miss out the ~whatever expansion here,
> 
> No, I get that for free.
> It's handled by glob().
> And it works, and it's pretty cool.
> I don't have a lot of use for it but I like it.

Ok, never knew glob did that, that's cool.

One thing puzzles me about the changes though:
 
/* But first see if the user wants to glob */
	if (varline[0] == '~') {
		char c = varline[1];
		if (!c)
			goto doglob;
		if (c == '/')
			goto doglob;
		if (isalphaByte(c))
			goto doglob;
	}
	for (s = varline; *s; ++s)
		if (strchr("*?[", *s) && (s == varline || s[-1] != '\\'))
			goto doglob;

noglob:
	*expanded = varline;
	return true;

I thought we were using globbing so that we didn't have nasty side-effects,
and I'm not entirely sure what this check solves.
If there are metacharacters in the expanded line by this point and they're
unescaped then we'll glob, otherwise they're correctly escaped and thus
globbing wouldn't do any harm anyway.
In addition glob appears to handle a ] without a [ etc,
so again this check seems somewhat redundant.
As for the r5 (or r<buffer_number>) problem which is what I guess this is
trying to solve, that number shouldn't have gone anywhere near this function
since, as far as I know, r<filename> is illegal and should be r <filename> anyway.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

The problem that the glob check solves is subtle,
and it only has to do with error messages.
Imagine you want to read a file into the current buffer.
You use the shorthand

r z*

because you believe this will grab the correct file.
But z* doesn't match anything.
Since we are globbing you get the error message

shell pattern doesn't match any files

or something like that.
Now if you're sure the file is called z*, or even zz,
and you type

r zz

you don't want to get that error message.
But that's what you get if you glob.
I know that you didn't want to glob, and didn't mean to glob,
and I give the other error message

cannot open file zz

Other than clear error messages corresponding to what you are trying to do,
the check probably doesn't change a thing.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

On Sat, Apr 18, 2015 at 09:09:34AM -0400, Karl Dahlke wrote:
> The problem that the glob check solves is subtle,
> and it only has to do with error messages.
> Imagine you want to read a file into the current buffer.
> You use the shorthand
> 
> r z*
> 
> because you believe this will grab the correct file.
> But z* doesn't match anything.
> Since we are globbing you get the error message
> 
> shell pattern doesn't match any files
> 
> or something like that.
> Now if you're sure the file is called z*, or even zz,
> and you type
> 
> r zz
> 
> you don't want to get that error message.
> But that's what you get if you glob.
> I know that you didn't want to glob, and didn't mean to glob,
> and I give the other error message
> 
> cannot open file zz
> 
> Other than clear error messages corresponding to what you are trying to do,
> the check probably doesn't change a thing.

I think I'd rather clean up the error message and remove the checking tbh,
something like:
"No files found matching z*"
That way we just glob and it works. Plus,
if I'm using variable expansion and globbing I get to see what I actually globbed.

At the moment, if I have a variable $filepath which I think is set to say
"/mnt/data/filestore/files/packages/descriptions/"
and I know that I've got a file called zfs_latest_version.txt under that
directory I'd probably type:
e $filepath/z*
Since I know that's the only file with that name under that directory.
However if I've changed $filepath to something else like "/mnt/data/filestore/files/patches/"
then I just get "Shell pattern did not match any files." which is rather unhelpful.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> I think I'd rather clean up the error message and remove the checking tbh,

I see where you're coming from.
These are behaviors, and even error messages, that have been this way for years,
and I'm ok with improving them, I just think I need to move carefully.
another message that isn't so great is

environment variable not defined

So if you edit $a$b and get this message,
is it a or be that is not defined?
Really, would it have been terribly hard for me
to include the variable in the error message?
Sometimes I wonder what I was thinking 5 or 10 years ago.

Karl Dahlke

Re: [Edbrowse-dev] wordexp again

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

On Sat, Apr 18, 2015 at 09:45:59AM -0400, Karl Dahlke wrote:
> > I think I'd rather clean up the error message and remove the checking tbh,
> 
> I see where you're coming from.
> These are behaviors, and even error messages, that have been this way for years,
> and I'm ok with improving them, I just think I need to move carefully.

I'm not so sure careful movement is so required here.
The only thing we may break is scripts,
and I doubt anyone was a fan of the "shell pattern did not match any files"
message in any case. At least this way the message (and code)
would be consistant and explanitory.
> another message that isn't so great is
> 
> environment variable not defined
> 
> So if you edit $a$b and get this message,
> is it a or be that is not defined?
> Really, would it have been terribly hard for me
> to include the variable in the error message?
> Sometimes I wonder what I was thinking 5 or 10 years ago.
I think everyone has these moments when looking at old code.
I'd like to see this message improved to include the variable though.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp again

[Karl Dahlke]

> I doubt anyone was a fan of the "shell pattern did not match any files"

Indeed, I'm just saying there is a process here.
If I change substantially an error message, or add a new one,
I have to add it to a queue of messages to be translated by my friends
into 4 other languages.
(And I'm hoping for more languages in the future.)
I don't want to pester them weekly with small changes,
nor do I want to hit them up with 50 messages to translate at a shot,
so when the queue has about ten or so, or there is a release coming up,
I send them the new messages for translation.
That's all fine, I'm just saying it's not just a matter
of changing a printf.
If I suddenly get hit by a meteor, their names and emails are in messages.c.

> environment variable is not defined
> ...
> I'd like to see this message improved to include the variable though.

This one I just pushed,
and it was simple enough that I could do it in all languages,
so no need to put it on the queue.
I could tell the words for undefined, and environment variable,
and I could turn it into something like $foo is not defined, in all languages,
even Polish, which is the least recognizable for me.
Sometimes I feel like I should be able to handle the German myself,
I almost can, but let's not get cocky,
because conversational German is not the same as technical German.

Karl Dahlke