Hi Bruno,
On 3/16/24 6:14 AM, Bruno Haible wrote:
> Oh, I had not seen that the use of 'classes.' was so inconsistent.
> Haven't made up my mind yet, regarding when 'classes.' should be used or not.
This is somewhat related since it deals with imports, but is there a
reason why we don't just import things from modules directly?
Right now we have:
from . import constants
# Define global constants.
APP = constants.APP
DIRS = constants.DIRS
joinpath = constants.joinpath
Typically I am used to seeing:
from .constants import APP
from .constants import DIRS
from .constants import joinpath
# or on one line
from .constants import APP, DIRS, joinpath
Is there a functional or stylistic reason for doing it as we have it
now?
> Still, there are two issues in the patch, more precisely in the last hunk
> of main.py:
>
> * if copymode != classes.CopyAction.Copy or lcopymode !=
> classes.CopyAction.Copy:
>
> The 'git update-index --refresh' command is only needed for hard links.
> Setting a symlink does not change the ctime of the target of the symlink.
Oops, yes my code disagrees with the comment there as well.
> if test -d "$gnulib_dir"/.git \
> && (git --version) >/dev/null 2>/dev/null; then
> (cd "$gnulib_dir" && git update-index --refresh >/dev/null)
> fi
>
> This means: If the 'git' program is not found, we don't run the command,
> because we know it would always fail. But other than that, we do print
> the errors from the subprocess. (There is no '2>/dev/null' here.)
Yes, that makes sense. I am slowly getting better at reading the shell
script stuff. That piece of code isn't too bad but I didn't spend
enough time looking at the redirections.
As far as the Python code goes, one annoyance that I have is that
sometimes it is hard to tell exactly which exceptions may be thrown by
a function. I'm not sure if it is frowned upon to catch all exceptions
blanketly, but I would typically do this as an example:
import subprocess as sp
import os
try:
sp.run(['invalid-command', 'update-index', '--refresh'],
cwd=os.getenv('GNULIB_REFDIR'), stdout=sp.DEVNULL, stderr=sp.DEVNULL)
except Exception as error:
print(error)
$ ./test.py
[Errno 2] No such file or directory: 'invalid-command'
But when executing 'git' instead of 'invalid-command' it executes
silently. But as you said, this is also the case for options that it
does not recognize. It should alert us if 'git' changes and does not
accept it.
For this code I can see two ways of doing this. We could just not
redirect stderr to match the gnulib-tool.sh script. Maybe 'git' will
give us warnings in stderr that are not necessarily failures.
Or we can use capture_output and check the return code, printing
stderr only if the command fails [1]:
import subprocess as sp
import os
try:
result = sp.run(['git', 'update-index', '--bad-option'],
cwd=os.getenv('GNULIB_REFDIR'), capture_output=True)
if result != 0:
print((result.stderr.decode(encoding='utf-8')))
except Exception as error:
print(error)
$ ./test.py
error: unknown option 'bad-option'
usage: git update-index [<options>] [--] [<file>...]
...
I am not sure what is the best option, but it is obvious that my
original code was incorrect. :)
I noticed that the gnulib-tool.py.TODO mentions using
sp.run(..., cwd=) more often. Once I get around to doing that, I will
check for more issues like this.
[1] https://docs.python.org/3/library/subprocess.html#
Collin
