zsh-workers
 help / color / mirror / Atom feed
* [BUG] zf_ln complains about the wrong argument
@ 2021-08-22 13:45 Marlon Richert
  2021-08-22 15:35 ` Mikael Magnusson
  2021-08-22 20:24 ` Peter Stephenson
  0 siblings, 2 replies; 7+ messages in thread
From: Marlon Richert @ 2021-08-22 13:45 UTC (permalink / raw)
  To: Zsh hackers list

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-22 13:45 [BUG] zf_ln complains about the wrong argument Marlon Richert
@ 2021-08-22 15:35 ` Mikael Magnusson
  2021-08-22 19:27   ` Marlon Richert
  2021-08-22 20:24 ` Peter Stephenson
  1 sibling, 1 reply; 7+ messages in thread
From: Mikael Magnusson @ 2021-08-22 15:35 UTC (permalink / raw)
  To: Marlon Richert; +Cc: Zsh hackers list

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-22 15:35 ` Mikael Magnusson
@ 2021-08-22 19:27   ` Marlon Richert
  0 siblings, 0 replies; 7+ messages in thread
From: Marlon Richert @ 2021-08-22 19:27 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: Zsh hackers list

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-22 13:45 [BUG] zf_ln complains about the wrong argument Marlon Richert
  2021-08-22 15:35 ` Mikael Magnusson
@ 2021-08-22 20:24 ` Peter Stephenson
  2021-08-22 21:05   ` Daniel Shahaf
  1 sibling, 1 reply; 7+ messages in thread
From: Peter Stephenson @ 2021-08-22 20:24 UTC (permalink / raw)
  To: zsh-workers

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-22 20:24 ` Peter Stephenson
@ 2021-08-22 21:05   ` Daniel Shahaf
  2021-08-23  8:33     ` Peter Stephenson
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Shahaf @ 2021-08-22 21:05 UTC (permalink / raw)
  To: zsh-workers

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-22 21:05   ` Daniel Shahaf
@ 2021-08-23  8:33     ` Peter Stephenson
  2021-08-24 13:04       ` Daniel Shahaf
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Stephenson @ 2021-08-23  8:33 UTC (permalink / raw)
  To: zsh-workers


> 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] zf_ln complains about the wrong argument
  2021-08-23  8:33     ` Peter Stephenson
@ 2021-08-24 13:04       ` Daniel Shahaf
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Shahaf @ 2021-08-24 13:04 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-24 13:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-22 13:45 [BUG] zf_ln complains about the wrong argument Marlon Richert
2021-08-22 15:35 ` Mikael Magnusson
2021-08-22 19:27   ` Marlon Richert
2021-08-22 20:24 ` Peter Stephenson
2021-08-22 21:05   ` Daniel Shahaf
2021-08-23  8:33     ` Peter Stephenson
2021-08-24 13:04       ` Daniel Shahaf

zsh-workers

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.vuxu.org/zsh-workers

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 zsh-workers zsh-workers/ https://inbox.vuxu.org/zsh-workers \
		zsh-workers@zsh.org
	public-inbox-index zsh-workers

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


code repositories for the project(s) associated with this inbox:

	https://git.vuxu.org/mirror/zsh/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git