[BUG] zf_ln complains about the wrong argument

[BUG] zf_ln complains about the wrong argument

[Marlon Richert]

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

Re: [BUG] zf_ln complains about the wrong argument

[Mikael Magnusson]

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

Re: [BUG] zf_ln complains about the wrong argument

[Marlon Richert]

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

Re: [BUG] zf_ln complains about the wrong argument

[Peter Stephenson]

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;
     }

Re: [BUG] zf_ln complains about the wrong argument

[Daniel Shahaf]

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.

Re: [BUG] zf_ln complains about the wrong argument

[Peter Stephenson]

> 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

Re: [BUG] zf_ln complains about the wrong argument

[Daniel Shahaf]

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