Assorted code and whitespace fixes

[solardiz]

This fixes or works around several issues I opened recently, and then drops oui.txt (for now), proceeds to make many whitespace fixes, and re-enables whitespace check in CI starting with a mid-2022 commit after which we introduced no unfixed problems like that.

[solardiz]

re-enables whitespace check in CI starting with a mid-2022 commit after which we introduced no unfixed problems like that.

Hmm, this fails in GitHub Actions:

Run git diff-index --check --cached b1b622f691d40196815939e4736a5da71befd206
fatal: bad object b1b622f691d40196815939e4736a5da71befd206

I don't know why, maybe @ldv-alt or @vt-alt does or even knows of a fix? I guess I'll have to omit this commit for now, but we can occasionally be running this command manually.

[vt-alt]

But why it was b1b622f691d40196815939e4736a5da71befd206instead of 4b825dc642cb6eb9a060e54bf8d69288fbee4904 (hash of empty tree object)?

[ldv-alt]

This is the hash I've been using in CI:

$ git hash-object -t tree --stdin < /dev/null 
4b825dc642cb6eb9a060e54bf8d69288fbee4904

[solardiz]

@vt-alt @ldv-alt Sorry, I should have explained. We had it at 4b825dc642cb6eb9a060e54bf8d69288fbee4904 initially, but there's no reasonable way we can make the entire tree meet the new whitespace requirements - e.g., there is a wordlist entry that looks like a leftover conflict marker, etc. b1b622f691d40196815939e4736a5da71befd206 is the oldest commit hash in our tree after which there are no new whitespace issues yet. Checking against it works locally, but not in GitHub Actions.

[ldv-alt]

The object has to be present, which is not necessarily the case when a shallow clone is used.

[vt-alt]

Also it should not be commit hash it's tree-ish, so you may try to extract tree hash from the commit object (with cat-file) or try b1b622f691d40196815939e4736a5da71befd206^{tree} (not sure though).

[vt-alt]

The object has to be present, which is not necessarily the case when a shallow clone is used.

https://github.com/actions/checkout
Only a single commit is fetched by default, for the ref/SHA that triggered the workflow. Set fetch-depth: 0 to fetch all history for all branches and tags.

So ldv is right.

[solardiz]

Thanks. With this addition, it works (tested under my personal GitHub account for now):

      with:
        fetch-depth: 0

BTW, we already have this for the build tests inherited from passwdqc - in there, I assume we have it for:

if git status --porcelain |grep ^.; then
	echo >&2 'git status reported uncleanness'
	exit 1
fi

but why do we need that? Maybe we should drop it?

[vt-alt]

Looks like good check (that there are no accidental modifications or untracked files). Any check could be dropped but then there is no check.

[solardiz]

good check (that there are no accidental modifications or untracked files)

Why would there be any in CI environment? Perhaps only if files are modified by build/tests, right? I don't mind checking, but if fetch-depth: 0 has significant cost I'm not sure this is worth it. Anyway, I'll leave this as-is for now.

[solardiz]

if git status --porcelain |grep ^.; then
	echo >&2 'git status reported uncleanness'
	exit 1
fi

Also, I'm unsure the above really needs more than just the latest commit to be fetched, does it?

[vt-alt]

Perhaps only if files are modified by build/tests, right?

Yes.

I'm unsure the above really needs more than just the latest commit to be fetched, does it?

1 commit is enough.

[solardiz]

1 commit is enough.

Is there a reason we have fetch-depth: 0 in all those tests, then? @ldv-alt I got all of this from your contribution to passwdqc (very helpful and reusable, as you can see - thank you), and I think you also have it that way in other projects.

[ldv-alt]

if git status --porcelain |grep ^.; then
	echo >&2 'git status reported uncleanness'
	exit 1
fi

Also, I'm unsure the above really needs more than just the latest commit to be fetched, does it?

git status needs just the current commit.

[ldv-alt]

1 commit is enough.

Is there a reason we have fetch-depth: 0 in all those tests, then? @ldv-alt I got all of this from your contribution to passwdqc (very helpful and reusable, as you can see - thank you), and I think you also have it that way in other projects.

I suppose it comes from strace.git where more git history is needed to generate some files.
If your CI doesn't need it, feel free to remove.

[ldv-alt]

Looks like good check (that there are no accidental modifications or untracked files). Any check could be dropped but then there is no check.

It's a good check for a project that maintains its .gitignore files.

[vt-alt]

I don't mind checking, but if fetch-depth: 0 has significant cost I'm not sure this is worth it.

Btw, I don't think 152M for JtR repo is a significant amount for an internal transfer from GA to GA infrastructure. (It's not even near to 6G of linux.git.) Much more is transferred when building from Docker images and package installs inside of them. (It's possible they use caches for this Docker stuff but this would be internal GA to GA transfer too.) I am unable to find in their actions/checkout repo reason why they make such default, it just appeared in 2019.

So I'd suggest to leave fetch-depth: 0 for everything.