regression? in cda8948b

[Qian Yun]

+ writable? f ==
+ n := f::String
+ if n = "" then
+ n := "."
+ r1 : Integer := writeablep(n)$Lisp
+ r1 > 0 => true
+ n := file_directory(n)$Lisp
+ r1 := writeablep(n)$Lisp
+ r1 > 0
-(defun |myWritable?| (s)
- (if (not (stringp s)) (|error| "``myWritable?'' requires a string arg."))
- (if (string= s "") (setq s "."))
- (if (not (|fnameExists?| s)) (setq s (|file_directory| s)))
- (if (string= s "") (setq s "."))
- (if (> (|writeablep| s) 0) 't nil) )

First, in "writable?", during the translation,
'if n = "" then n := "."' is lost after the 'file_directory' call.

This causes 'writable?("non-exist-file")$FNAME' to return false
instead of true.


+MAKEPROP('coerce, "/TRANSFORM", ["&", "&", "*"])
+MAKEPROP('comp, "/TRANSFORM", ["&", "*", "*", "&"])
+MAKEPROP('compIf, "/TRANSFORM", '["&", "*", "*", "&"])
+MAKEPROP('compFormWithModemap, "/TRANSFORM", ["&", "*", "*", "&", "*"])

Second, the quote in 'compIf' line is unintentional?

- Qian

[Qian Yun]

Ping?

[Waldek Hebisch]

On Thu, Apr 23, 2026 at 07:53:36PM +0800, Qian Yun wrote:
> + writable? f ==
> + n := f::String
> + if n = "" then
> + n := "."
> + r1 : Integer := writeablep(n)$Lisp
> + r1 > 0 => true
> + n := file_directory(n)$Lisp
> + r1 := writeablep(n)$Lisp
> + r1 > 0
> -(defun |myWritable?| (s)
> - (if (not (stringp s)) (|error| "``myWritable?'' requires a string arg."))
> - (if (string= s "") (setq s "."))
> - (if (not (|fnameExists?| s)) (setq s (|file_directory| s)))
> - (if (string= s "") (setq s "."))
> - (if (> (|writeablep| s) 0) 't nil) )
>
> First, in "writable?", during the translation,
> 'if n = "" then n := "."' is lost after the 'file_directory' call.
>
> This causes 'writable?("non-exist-file")$FNAME' to return false
> instead of true.

Yes, we probably should add "." here too. Or change 'writeabolep'
to do this.

>
> +MAKEPROP('coerce, "/TRANSFORM", ["&", "&", "*"])
> +MAKEPROP('comp, "/TRANSFORM", ["&", "*", "*", "&"])
> +MAKEPROP('compIf, "/TRANSFORM", '["&", "*", "*", "&"])
> +MAKEPROP('compFormWithModemap, "/TRANSFORM", ["&", "*", "*", "&", "*"])
>
> Second, the quote in 'compIf' line is unintentional?

No, there should be no quote.

--
Waldek Hebisch

[Qian Yun]

On 4/30/26 9:05 PM, Waldek Hebisch wrote:
> On Thu, Apr 23, 2026 at 07:53:36PM +0800, Qian Yun wrote:
>> + writable? f ==
>> + n := f::String
>> + if n = "" then
>> + n := "."
>> + r1 : Integer := writeablep(n)$Lisp
>> + r1 > 0 => true
>> + n := file_directory(n)$Lisp
>> + r1 := writeablep(n)$Lisp
>> + r1 > 0
>> -(defun |myWritable?| (s)
>> - (if (not (stringp s)) (|error| "``myWritable?'' requires a string arg."))
>> - (if (string= s "") (setq s "."))
>> - (if (not (|fnameExists?| s)) (setq s (|file_directory| s)))
>> - (if (string= s "") (setq s "."))
>> - (if (> (|writeablep| s) 0) 't nil) )
>>
>> First, in "writable?", during the translation,
>> 'if n = "" then n := "."' is lost after the 'file_directory' call.
>>
>> This causes 'writable?("non-exist-file")$FNAME' to return false
>> instead of true.
>
> Yes, we probably should add "." here too. Or change 'writeabolep'
> to do this.
>

There is another issue: old code uses "fnameExists?", but new code
use "writeablep". So if a file exists but not writable (aka readonly),
new code will test directory instead, and may return true.

See attachment for a patch.

- Qian

[Waldek Hebisch]

writeablep has info from 'stat' and ideally just calling it
should work. I definitely dislike idea of doing extra call to
'exists?'.

BTW: Main motivation for this and releated changes to get sane
semantics. Orignal code used CMS semantics and later ports
added emulatation of CMS featurs. Upper level wanted
different semantics, so added its own processing. But we
really should have one level of functions on top of OS
calls, mutiple layers of adapeters definitely make file
handling more complex.

> diff --git a/src/algebra/fname.spad b/src/algebra/fname.spad
> index 15285546..03d46969 100644
> --- a/src/algebra/fname.spad
> +++ b/src/algebra/fname.spad
> @@ -87,12 +87,11 @@ FileName() : FileNameCategory == add
> n := f::String

> if n = "" then
> n := "."

> - r1 : Integer := writeablep(n)$Lisp
> - r1 > 0 => true
> - n := file_directory(n)$Lisp
> + if not exists?(n::%) then
> + n := directory(n::%)

> if n = "" then
> n := "."

> - r1 := writeablep(n)$Lisp
> + r1 : Integer := writeablep(n)$Lisp
> r1 > 0
>
> new(d, pref, e) ==


--
Waldek Hebisch

[Qian Yun]

Do you think there's a problem with current implementation
of "writable?"? I'm a bit confused.

- Qian

[Waldek Hebisch]

On Sat, May 16, 2026 at 09:07:40PM +0800, Qian Yun wrote:
> Do you think there's a problem with current implementation
> of "writable?"? I'm a bit confused.

The point is to simplify things. We should look at uses of
'writeablep' and make sure that if has desired effect for
'writable?'.

> --
> You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel...@googlegroups.com.
> To view this discussion visit https://groups.google.com/d/msgid/fricas-devel/e79b6aab-5169-411e-b34e-889e9bf6565f%40gmail.com.

--
Waldek Hebisch

[Qian Yun]

(1) -> )system touch filea
(1) -> )system chmod a-w filea
(1) -> writable? "filea"

(1) true

Just to be clear, this is not the expected result, right?

- Qian

[Waldek Hebisch]

On Sat, May 16, 2026 at 10:00:48PM +0800, Qian Yun wrote:
> (1) -> )system touch filea
> (1) -> )system chmod a-w filea
> (1) -> writable? "filea"
>
> (1) true
>
> Just to be clear, this is not the expected result, right?

Yes, this is wrong. My point is that correct version should
be much simpler. For this case recursing to directory only
when 'writablep' returns -1 should fix the problem, but
really either 'writablep' should not recurse to directory
or do it in way that we need, without the extra hacks.

> - Qian
>
> On 5/16/26 9:54 PM, Waldek Hebisch wrote:
> > On Sat, May 16, 2026 at 09:07:40PM +0800, Qian Yun wrote:
> >> Do you think there's a problem with current implementation
> >> of "writable?"? I'm a bit confused.
> >
> > The point is to simplify things. We should look at uses of
> > 'writeablep' and make sure that if has desired effect for
> > 'writable?'.
> >
>

> --
> You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel...@googlegroups.com.

> To view this discussion visit https://groups.google.com/d/msgid/fricas-devel/1f853c3d-9d93-42d1-ba6a-382a45b46581%40gmail.com.

--
Waldek Hebisch

[Waldek Hebisch]

On Sat, May 16, 2026 at 10:00:48PM +0800, Qian Yun wrote:

> (1) -> )system touch filea
> (1) -> )system chmod a-w filea
> (1) -> writable? "filea"
>
> (1) true
>
> Just to be clear, this is not the expected result, right?

AFAICS the attached patch fixes the problem. Trouble was that
'writablep' was doing different thing than it promised. Current
version should fix this.

BTW: The only user of 'writablep' is 'writable?' in fname.spad.
For that use distinct return code for files and directores is
not needed, we could just return 0 or 1 (which will be turned
into Boolean anyway). But for minimal change I keep original
return convention.

--
Waldek Hebisch

[Qian Yun]

Replies from LLM (verified by me as well):

`writable?` returns false positives for files in the root
directory (e.g., `"/foo"`)

Because the loop condition is `c != t` and the loop body
executes *after* the condition is checked, it never evaluates
the first character `t[0]`.

For a path like `"/foo"`, the only slash is at `t[0]`.
The loop will finish without finding a slash, leaving `c`
equal to `t` and `pos` as `NULL`.

Then it mistakenly sets the parent directory to `"."`
(the current working directory) instead of `"/"` (the root directory).

Recommendation to fix:

- Use `if (pos == NULL)` to determine if a slash was found,
instead of `c == t`.

- Modify the loop logic to ensure `t[0]` is checked (e.g.,
by changing the bounds, but being careful not to decrement
the pointer before the array starts if checking `c >= t`).

- If the slash is found at `t[0]` (i.e., `pos == t`),
ensure that the resulting directory is `"/"` and not an empty
string (which would occur if you simply performed `strncpy(s, t, 0)`).

- Qian