When the second argument to zf_ln is an empty string, zf_ln mistakenly reports that the first argument is a non-existing file or dir: % zmodload zsh/files % touch foo % zf_ln foo '' zf_ln: foo: no such file or directory
On 8/22/21, Marlon Richert <marlon.richert@gmail.com> wrote:
> When the second argument to zf_ln is an empty string, zf_ln mistakenly
> reports that the first argument is a non-existing file or dir:
>
> % zmodload zsh/files
> % touch foo
> % zf_ln foo ''
> zf_ln: foo: no such file or directory
As do zf_mv, gnu ln, and gnu mv.
--
Mikael Magnusson
On Sun, Aug 22, 2021 at 6:35 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 8/22/21, Marlon Richert <marlon.richert@gmail.com> wrote:
> > When the second argument to zf_ln is an empty string, zf_ln mistakenly
> > reports that the first argument is a non-existing file or dir:
> >
> > % zmodload zsh/files
> > % touch foo
> > % zf_ln foo ''
> > zf_ln: foo: no such file or directory
>
> As do zf_mv, gnu ln, and gnu mv.
Well, two wrongs don't make a right, don't you think? Just because Gnu
prints confusing error messages, doesn't mean Zsh can't do better.
Plus, BSD ln does manage to complain about the right file:
% ln foo ''
ln: : No such file or directory
Also, I discovered this happens, too, when trying to create a symlink
in a dir that doesn't exist:
% zf_ln foo bar/baz
zf_ln: foo: no such file or directory
BSD ln again gives a better error message (although still not 100% accurate):
% ln foo bar/baz
ln: bar/baz: No such file or directory
On Sun, 2021-08-22 at 16:45 +0300, Marlon Richert wrote: > When the second argument to zf_ln is an empty string, zf_ln mistakenly > reports that the first argument is a non-existing file or dir: > > % zmodload zsh/files > % touch foo > % zf_ln foo '' > zf_ln: foo: no such file or directory Is this good enough to detect cases like this? An extra stat in the error case should be neither here nor there. Also seems a good idea to turn a null string into a couple of quotes. I thought of modifying the error message handler but this has too many knock on effects. pws diff --git a/Src/Modules/files.c b/Src/Modules/files.c index a1d6f6bf2..d4ec93624 100644 --- a/Src/Modules/files.c +++ b/Src/Modules/files.c @@ -346,7 +346,16 @@ domove(char *nam, MoveFunc movefn, char *p, char *q, int flags) unlink(qbuf); } if(movefn(pbuf, qbuf)) { - zwarnnam(nam, "%s: %e", p, errno); + int ferrno = errno; + char *errfile = p; + if (ferrno == ENOENT && !lstat(pbuf, &st)) { + /* p *does* exist, so error is in q */ + errfile = q; + } + if (*errfile) + zwarnnam(nam, "%s: %e", errfile, ferrno); + else + zwarnnam(nam, "`': %e", ferrno); zsfree(pbuf); return 1; }
Peter Stephenson wrote on Sun, 22 Aug 2021 20:24 +00:00:
> Also seems a good idea to turn a null string into a couple of quotes. I
> thought of modifying the error message handler but this has too many
> knock on effects.
What "knock on effects"? Extending zwarning() with a %q format code
that takes a «char *» and outputs it quoted seems like a good idea and
shouldn't break anything. Will handle filenames with spaces, too.
> On 22 August 2021 at 22:05 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Peter Stephenson wrote on Sun, 22 Aug 2021 20:24 +00:00:
> > Also seems a good idea to turn a null string into a couple of quotes. I
> > thought of modifying the error message handler but this has too many
> > knock on effects.
>
> What "knock on effects"? Extending zwarning() with a %q format code
> that takes a «char *» and outputs it quoted seems like a good idea and
> shouldn't break anything. Will handle filenames with spaces, too.
This is certainly all stuff that's accumulated rather than been planned
over the years, but having a harder look I think it's mostly not too bad
the way it is. I think the basics are currently as follows.
%s as implemented in errors and warnings is already not a straight
printf %s: it uses nicezputs() to emphasise visibility rather than quoting,
i.e. it's there to make it easy to see what the error is about, rather than
easy to copy back into the shell. Given the error is about something that
would usually be either in a script or on the command line, this doesn't
seem a bad way of doing it, on reflection.
As far as quoting is concerned, the state of the art is to use `%s' (i.e.
with those literal quotes) if it seems necessary to draw attention to the
argument, i.e. it could get lost otherwise. There are a lot of cases
where we don't do this, however, but they can easily be changed. The
combination of this and nicezputs() means you can always see what's
there, unless there are very subtle cases I've missed.
So I think the consistent way of handling a case like this is to turn
%s into `%s'. That both makes it visible what's going on and makes
the output consistent with similar cases elsewhere.
Does that seem reasonable? I can't think of a strong argument for a
more widespread change given it's sort of working and even sort of
potentially consistent.
pws
Peter Stephenson wrote on Mon, Aug 23, 2021 at 09:33:21 +0100: > > > On 22 August 2021 at 22:05 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > Peter Stephenson wrote on Sun, 22 Aug 2021 20:24 +00:00: > > > Also seems a good idea to turn a null string into a couple of quotes. I > > > thought of modifying the error message handler but this has too many > > > knock on effects. > > > > What "knock on effects"? Extending zwarning() with a %q format code > > that takes a «char *» and outputs it quoted seems like a good idea and > > shouldn't break anything. Will handle filenames with spaces, too. > > This is certainly all stuff that's accumulated rather than been planned > over the years, but having a harder look I think it's mostly not too bad > the way it is. I think the basics are currently as follows. > > %s as implemented in errors and warnings is already not a straight > printf %s: it uses nicezputs() to emphasise visibility rather than quoting, > i.e. it's there to make it easy to see what the error is about, rather than > easy to copy back into the shell. Given the error is about something that > would usually be either in a script or on the command line, this doesn't > seem a bad way of doing it, on reflection. > Well, sure, %s doesn't have to use the an escaping algorithm that's inputtable back into the shell, but it'd still be nice if the escaping were clear and unambiguous. As it stands, printing «%s» doesn't escape or quote empty strings and spaces. > As far as quoting is concerned, the state of the art is to use `%s' (i.e. > with those literal quotes) if it seems necessary to draw attention to the > argument, i.e. it could get lost otherwise. There are a lot of cases > where we don't do this, however, but they can easily be changed. The > combination of this and nicezputs() means you can always see what's > there, unless there are very subtle cases I've missed. If the argument to %s contains literal single quotes then this is ambiguous. > So I think the consistent way of handling a case like this is to turn > %s into `%s'. That both makes it visible what's going on and makes > the output consistent with similar cases elsewhere. > > Does that seem reasonable? +1 > I can't think of a strong argument for a more widespread change given > it's sort of working and even sort of potentially consistent. Well, if there were a %q that did unambiguous escaping/quoting, it'd be harder to accidentally write zwarning("foo: %s: bar") in new code, forgetting to quote. And of course, filenames with single quotes would be output more unambiguously :) Cheers, Daniel