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

[Greg Chicares]

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

Locally, I don't see it:

/opt/lmi/src/lmi[0]$ls -l .git/info/exclude .git/description
-rw-r--r-- 1 greg greg  73 Sep 12  2019 .git/description
-rw-r--r-- 1 greg greg 240 Sep 12  2019 .git/info/exclude

At savannah.org...
  https://git.savannah.nongnu.org/cgit/lmi.git/tree/
...I can't see it at all because no '.git' subdirectory is shown.

IIRC, '.git/description' was generated automatically when
the savannah administrators created our git repository.
Indeed, locally here it contains:
  "Unnamed repository; edit this file 'description' to name the repository."
and, also IIRC, we might edit that locally, but that's
pointless because we cannot push it to savannah.

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

>  It is indeed surprising that these files are executable, but they indeed
> are, as could be confirmed by just running "ls -l .git", as shown a bit
> higher in the output of the same job.

But I ran that command, above, and it says they're not executable here.

>  I have a few solutions here, but neither seems ideal:
> 
> 0. Don't do anything, just ignore these warnings.
> 
>    I am not sure if it's actually normal that check_script.sh warns without
>    giving an error or if it's just an oversight -- I'd prefer if the script
>    was more certain about its conclusions and exited with an error, rather
>    than complaining not too intrusively. It's also annoying to see these
>    warnings in the logs, so I'd rather get rid of them.

That's a philosophical issue that we've discussed before.
I prefer warn-and-continue (because it gives me other
warnings that occur downstream) to warn-and-abort (because
it doesn't). Sometimes a lengthy job runs perfectly except
for some trivial and isolated warning, and I that's
information that I want to know.

IIRC, you prefer the latter because it returns an error code.

Both points of view have their pro's and con's, but either
we abort or we do not abort.

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

> 1. Ignore these files in check_script.sh, just as it's already done for
>    .git/hooks-orig*.

As that '.sh' documents, it skips default hooks because...

#  - git's default hooks: the maintainers don't use 'shellcheck'

Of course, preserving git's default hooks in hooks-orig is
somewhere between a nicety and an extravagance--you can remove
it if you don't want them preserved, but I think it would be
beyond extravagant to spend time making those hooks
shellcheck-clean.

>    This seems a bit too ad hoc, I have no idea why are these particular
>    files executable in GitHub Actions environment, and so am not sure at
>    all that other files under .git might not become executable too one
>    day.

I wonder whether this might simply be a github defect. That seems
to be the likeliest explanation of the evidence I can see, so it
is my working hypothesis.

If it's a defect that demonstrably exists on savannah, and we
can fix it on savannah, then that's the right thing to do.

> 2. Ignore all files under .git in GNUmakefile, i.e. replace the existing
>    "-path .git/modules -prune" with "-path .git" prune.
[...]
>    So this is the solution I prefer, but I also suspect that it might be
>    the one you like the least. Could I be wrong about it?

I like this least. AFAICS, it amounts to willfully ignoring something
that exists, only because github defectively mangles it.

> 3. Just run "chmod -x" on the relevant files in the CI script itself.
> 
>    This is what I did for now (in a not yet merged PR), because it solves
>    the immediate problem and doesn't touch lmi code, but I don't like it
>    all because it combines the drawbacks of (2) and (3).

But if this is indeed, as I conjecture, a github defect,
then fixing it only on github, with a github-only "action",
is the next best thing to having github fix its error.
For that reason (and again assuming my conjecture is correct)...

>  So ideally, I think, we should:
> 
> (a) Make check_script.sh return an error, not just log a warning, if
>     something is wrong, so that problems have no chance of going unnoticed.
> 
> (b) Fix makefile or script to skip the files under .git.

...I would therefore prefer (3) above to (a) or (b).