Skip to content

Commit

Permalink
No more 0 and 0 or 1
Browse files Browse the repository at this point in the history
  • Loading branch information
@jlaurens
jlaurens committed Jul 4, 2024
1 parent 11829b2 commit a4cfc46
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions l3build-file-functions.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,12 @@ function cp(glob, source, dest)
errorlevel = execute(
'xcopy /y /e /i "' .. unix_to_win(p.cwd) .. '" '
.. unix_to_win(dest .. '/' .. escapepath(p.src)) .. ' > nul'
) and 0 or 1
) -- execute returns an integer

This comment has been minimized.

Sign in to view

Copy link
@davidcarlisle

davidcarlisle Jul 4, 2024

Member

I believe the intention here was to ensure that only 0 or 1 is returmed rather than rely on system-dependent error codes
else
errorlevel = execute(
'xcopy /y "' .. unix_to_win(p.cwd) .. '" '
.. unix_to_win(dest .. '/') .. ' > nul'
) and 0 or 1
) -- execute returns an integer
end
else
-- Ensure we get similar behavior on all platforms
Expand All @@ -251,7 +251,7 @@ function cp(glob, source, dest)
end
errorlevel = execute(
"cp -RLf '" .. p.cwd .. "' " .. dest
) and 0 or 1
) -- execute returns an integer
end
if errorlevel ~=0 then
return errorlevel
Expand Down

3 comments on commit a4cfc46

@jlaurens
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper code to ensure 0 or 1 is

execute() == 0 and 0 or 1

because execute returns only integers such that execute(...) and 0 or 1 never returns 1.

Why is the 0|1 return code necessary here but not for others

@davidcarlisle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes the fact that the existing code didn't work as intended is another issue.

Question is whether we should fix the bug while preserving the original intention or as you suggest here to drop it completely
As you say, other functions expose the os.execute return value .

I don't know if @josephwright remembers why these functions had the ... or 1 added?

@josephwright
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have places where we get a return value of 256 from some successful processes, and those need to be coerced to give 0 - I don't remember if this is one but we do need to watch out for that.

Please sign in to comment.