Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 58.

[Bruno Haible]

Hi Collin,

> Let me know how this looks. I've imported CopyAction to main and use
> that in a similar way to what you wrote.

Nice work. Thanks! Applied.

> I've used the name copymode instead of copyaction to match
> gnulib-tool.sh.

Yes, good. I had forgotten about the distinction between copymode and
copyaction.

> > * GLFileSystem.py:
> > 
> >                     constants.hardlink(lookedup, joinpath(destdir, 
> > rewritten))
> > 
> >   Please import the 'hardlink' symbol, so that you can reference it like 
> > this:
> > 
> >                     hardlink(lookedup, joinpath(destdir, rewritten))
> 
> I didn't do this since the invocations of link_if_changed and substart
> used the 'constants.' prefix. Since you wanted this one change I have
> gone ahead and done the rest of them.
> 
> In main(), all of the classes are prefixed with 'classes.'. Because
> there are many occurrences of it, I have stuck to that convention for
> now.

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.

> I rather submit another patch later fixing that.

Yes, later. This is not urgent.

> >     -S | --more-symlinks
> >     -H | --more-hardlinks
> >   But it doesn't do so.
> 
> These are just aliases for the other options correct?
> 
> I've added them to the corresponding add_argument call like this:
> 
>      parser.add_argument('-h', '-H', '--hardlink', '--more-hardlinks',
>                          ...)  # rest
> 
> And similarly for --symlink.

Correct. Thanks.

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.

* Exception handling:

            try:
                sp.run(['git', 'update-index', '--refresh'],
                       cwd=APP['root'], stdout=sp.DEVNULL, stderr=sp.DEVNULL)
            except Exception:
                # We did our best...
                pass

  Catching *all* kind of exceptions *without* some logged feedback is a no-no.
  Because such code is not future-proof: If, say, the 'git' command stops
  accepting the '--refresh' option in 10 years, this subprocess invocation
  will always fail, and no one will notice it.

  Either catch only the specific type of exceptions. Or print some non-fatal
  error message if it failed.

  You'll notice how it was done in gnulib-tool.sh:

  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.)

Bruno