Re: [lmi] Minor problem with checking executable files under .git

[Vadim Zeitlin]

On Tue, 23 Mar 2021 22:53:07 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 3/21/21 8:10 PM, Vadim Zeitlin wrote:
GC> > 
GC> >  I have a minor problem with the CI builds, which was probably there since
GC> > the beginning but which I hadn't noticed before: running "make
GC> > check_concinnity" there results in a couple of warnings:
GC> > 
GC> >   file .../lmi/.git/info/exclude is executable, but should it be?
GC> >   file .../lmi/.git/description is executable, but should it be?
GC> > 
GC> > see 
https://github.com/let-me-illustrate/lmi/runs/2160993006?check_suite_focus=true#step:20:39
GC> 
GC> Locally, I don't see it:

 Sorry for being unclear. This is definitely and without doubt something
peculiar to GitHub Actions environment or, as you say, its defect. We can't
change it, however, so understanding this is nice but doesn't really help.

GC> I don't find those files with this command:
GC>   git ls-files --stage

 This is perfectly normal, they're internal Git files and not part of the
repository itself.

GC> >  I have a few solutions here, but neither seems ideal:
GC> > 
GC> > 0. Don't do anything, just ignore these warnings.
GC> > 
GC> >    I am not sure if it's actually normal that check_script.sh warns 
without
GC> >    giving an error or if it's just an oversight -- I'd prefer if the 
script
GC> >    was more certain about its conclusions and exited with an error, rather
GC> >    than complaining not too intrusively. It's also annoying to see these
GC> >    warnings in the logs, so I'd rather get rid of them.
GC> 
GC> That's a philosophical issue that we've discussed before.
GC> I prefer warn-and-continue (because it gives me other
GC> warnings that occur downstream) to warn-and-abort (because
GC> it doesn't). Sometimes a lengthy job runs perfectly except
GC> for some trivial and isolated warning, and I that's
GC> information that I want to know.

 Yes, I believe that warnings that can be ignored shouldn't exist.

GC> Would you consider capturing the output and filtering out
GC> everything that's unexpected? Then anything that wasn't
GC> filtered can be considered erroneous.

 Yes, I could do this. I could also just ignore these warnings completely,
of course, if you are not bothered by them.

GC> > 1. Ignore these files in check_script.sh, just as it's already done for
GC> >    .git/hooks-orig*.
GC> 
GC> As that '.sh' documents, it skips default hooks because...
GC> 
GC> #  - git's default hooks: the maintainers don't use 'shellcheck'
GC> 
GC> Of course, preserving git's default hooks in hooks-orig is
GC> somewhere between a nicety and an extravagance--you can remove
GC> it if you don't want them preserved, but I think it would be
GC> beyond extravagant to spend time making those hooks
GC> shellcheck-clean.

 Absolutely. And this is because they don't "belong" to us, they're Git's
own files and we don't, and shouldn't, really care about their style (or
lack of it) at all.

 And to me it seems very, very clear that this applies to all the other
files under .git, with possible exceptions for the individual files that we
know belong to us there, too. I clearly didn't manage to explain this in my
first message, so please let me try again: files under .git belong to Git,
in particular they can change at any moment (e.g. new version of Git
regularly add new files and directories there), including becoming
executable, and we should treat .git as (almost) completely opaque and
never do anything directly with it. Creating hook files under .git/hooks is
one of the very few exceptions to this rule and can justify checking the
particular files that we create there with spellcheck (although even this
is unnecessary, as they're just copies of the files we already check).

 Finally, if the above is somehow insufficiently convincing, I'd also like
to notice that recursing into .git, and .git/objects, is also fairly
inefficient as this directory contains all the repository data (i.e. full
history of everything) and a lot of files and subdirectories. So not only
is this completely unnecessary, but it's also inefficient to do it.


GC> > 2. Ignore all files under .git in GNUmakefile, i.e. replace the existing
GC> >    "-path .git/modules -prune" with "-path .git" prune.
GC> [...]
GC> >    So this is the solution I prefer, but I also suspect that it might be
GC> >    the one you like the least. Could I be wrong about it?
GC> 
GC> I like this least. AFAICS, it amounts to willfully ignoring something
GC> that exists, only because github defectively mangles it.

 No, this is really not at all what I was saying. It amounts to willfully
ignoring something because it's the right thing to do because we shouldn't
be even looking at Git privates, let alone shell-checking them. The fact
that GitHub does something weird is completely incidental and was just the
trigger that made me notice that we were doing this. If I had noticed it
otherwise, I would still have argued against doing it.

 Of course, it's a minor problem, so we shouldn't spend too much time
discussing it, but I'm just frustrated by the fact that my explanations
clearly didn't work at all. Please let me know if I managed to explain it
better this time.


GC> > 3. Just run "chmod -x" on the relevant files in the CI script itself.
GC> > 
GC> >    This is what I did for now (in a not yet merged PR), because it solves
GC> >    the immediate problem and doesn't touch lmi code, but I don't like it
GC> >    all because it combines the drawbacks of (2) and (3).
GC> 
GC> But if this is indeed, as I conjecture, a github defect,
GC> then fixing it only on github, with a github-only "action",
GC> is the next best thing to having github fix its error.
GC> For that reason (and again assuming my conjecture is correct)...
GC> 
GC> >  So ideally, I think, we should:
GC> > 
GC> > (a) Make check_script.sh return an error, not just log a warning, if
GC> >     something is wrong, so that problems have no chance of going 
unnoticed.
GC> > 
GC> > (b) Fix makefile or script to skip the files under .git.
GC> 
GC> ...I would therefore prefer (3) above to (a) or (b).

 Again, this is not important enough, but I'd like to reemphasize that the
rationale for doing (2) is to Do The Right Thing in lmi (or, more
practically speaking, isolate check_concinnity from by any future Git
changes that could break it and also make it a bit faster), avoiding the
warnings on GitHub is incidental.

 Please let me know if this changes anything for you, thanks in advance,
VZ

pgp0MY35KQqiL.pgp
Description: PGP signature