Tell github-linguist to count fixture shell scripts #1607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 24, 2024
Byron
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 24, 2024
This was brought up in GitoxideLabs#1607 and I think it's preferable.
Byron
approved these changes
Sep 24, 2024
There was a problem hiding this comment.
Thanks a lot for contributing this improvement, which happens to also have exposed a very annoying shortcoming that I have also encountered.
Instead of working around it like last time, I thought it's best to just hit it with an axe, and let CI run more occasionally instead of trying to be very smart with path-based rules.
From a convenience standpoint, it will be very much the same as auto-merge does the important work, with the notable disadvantage that some PRs could be merged faster.
|
It seems the test-failure is legit even though I restarted it to be very sure. It is probably because somehow, the changed @EliahKagan, can you reproduce this on Windows? Since the tests never ran, maybe it's unrelated to the Thanks for your help. |
Sounds good. I suggest keeping the
Maybe I was mistaken to think But these failures, I recognize. They are the same tests shown in #1358 to currently fail on Windows when all fixture scripts have to be run due to So generated archives are not usable, for some reason, after these changes, at least on Windows. (In addition to these being the same failing tests as in #1358, they seems to be failing in the same way and, in particular, the detailed test output includes messages about how the fixture scripts have to run.) I'll try it without the changes in dd94f57 and see what happens. |
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 24, 2024
This reverts commit dd94f57. One of the changes to `.gitattributes`, possibly that change, has caused the currently committed generated archives not to be able to be used, producing Windows `test-fast` failures similar to GitoxideLabs#1358. See comments in GitoxideLabs#1607 for details.
auto-merge was automatically disabled
September 24, 2024 20:44
Head branch was pushed to by a user without write access
|
That worked. The Windows Since I'm amending anyway to clarify the revert commit message, I'm also going ahead and rebasing to avoid confusion between a48965c (#1609) and what is currently fc06e41 here, as I suggested in #1607 (comment) could help clarify the history. The presence of the commit being reverted, as well as the revert commit with the descriptive message, seems like valuable enough history that I do not plan to rewind them away. |
This was brought up in GitoxideLabs#1607 and I think it's preferable.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 24, 2024
This reverts commit dd94f57. That change to `.gitattributes`, though intended as a refactoring, caused the currently committed generated archives not to be able to be used, producing Windows `test-fast` failures similar to GitoxideLabs#1358. See comments in GitoxideLabs#1607 for details.
EliahKagan
force-pushed
the
count-sh
branch
from
a403510
to
f53242a
Compare
|
Apparently the leading When they have such line terminators, Git Bash is still able to run them, because it has functionality added downstream to do so, and the effect of running them would still usually be the same. The breakage occurred not from changed behavior of the scripts, but instead because, due to having different line endings, the scripts' CRC32 checksums were different, and therefore the committed archives generated from them could not be used. All fixture scripts therefore had to be rerun, leading to the failures exactly as described in #1358 (except for the one performance test that was already known not always to fail). The cause of the problem is that patterns in
That documents it for
Although one of those exceptions is, as noted, relevant in practice to some patterns using To figure out what caused the behavior described in #1607 (comment), #1607 (comment), and #1607 (comment), I checked out all files anew on Windows by creating two new worktrees: Then, in Git Bash--just because it is easier to do there (it is not the cause of the problem)--I checked what the Unix Note the Interestingly, some of the scripts that are not fixtures are checked out with CRLF line endings on Windows and, presumably, have been checked out that way for some time. This should be fixed, but its consequences are minimal because Git Bash supports running such scripts and those particular scripts can have different hashes without affecting the tests. This is something that I think should be fixed, but I suggest it be fixed in a separate PR. I suspect |
This reverts commit a75e970. That change to `.gitattributes`, though intended as a refactoring, caused the currently committed generated archives not to be able to be used, producing Windows `test-fast` failures similar to GitoxideLabs#1358. See comments in GitoxideLabs#1607 for details.
EliahKagan
force-pushed
the
count-sh
branch
from
f53242a
to
d3c3237
Compare
Byron
approved these changes
Sep 25, 2024
There was a problem hiding this comment.
The cause of the problem is that patterns in
.gitignoreor.gitattributesare only "floating" when they have no non-trailing/. Even a/in the middle causes the pattern to be matched only relative to the location of the.gitignoreor.gitattributesfile that contains it. From gitignore(5) in the pattern format section:
That's a wonderful find! I also should know better given I implemented both of these matching engines 😁 - but of course the little special case I wasn't aware of anymore. It's really nice of the Git documentation to summarize it like that though.
Interestingly, some of the scripts that are not fixtures are checked out with CRLF line endings on Windows and, presumably, have been checked out that way for some time. This should be fixed, but its consequences are minimal because Git Bash supports running such scripts and those particular scripts can have different hashes without affecting the tests. This is something that I think should be fixed, but I suggest it be fixed in a separate PR. I suspect
.gitattributescan be simplified, in such a way as to satisfy the spirit of dd94f57/a75e970, which I suggest be done separately from this PR as well.
That's a great find, and I like how worktrees are used to easily examine different states of the repository without changing the local worktree.
And yes, let's fix line-endings in a separate PR - I suppose that would be a good occasion to use renormalization.
In any case, this PR is ready to go.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default, github-linguist treats source code files inside
tests/fixturesdirectories as vendored, thereby declining to consider them when computing the percentage each language contributes to a repository:But in this repository, the shell scripts inside
tests/fixturesare code that is part of the test suite. The effect of treating them as vendored is that only the comparably few shell scripts that are not test fixture scripts contribute to the language computation (and possibly other uses of github-linguist by various tooling? I am not sure).Thus, the "Languages" bar on GitHub for this repository (or in forks) currently shows 97.5% Rust, 1.3% HTML, and 1.2% Other. But "Shell" should really be shown as a the second most prevalent language here, with a percentage significantly exceeding that of HTML.
Running
github-linguist -blocally verifies the situation.Marking the affected files as not vendored, with the usual pattern we have been using to adjust their attributes in other ways, in the top-level
.gitattributesfile, fixes that. This PR makes that change.This changes the output of
github-linguistfrom:To:
I have verified that
github-linguistidentifies all the expected files by running:(Where the reason for
-printf '%P\n'was to omit the leading./that would otherwise stymiediff. I don't know if this is portable; I used GNU find.)On the
count-shbranch--the feature branch for this PR--that gives no output.Regarding non-obvious specifics of the edit to
.gitattributes:**/tests/fixtures/**/*.shconfers a benefit overtests/fixtures/**/*.sh, I've used it because it was already in use.linguist-vendored=false(or-linguist-vendored) could be added to the previous line with this pattern, but I added a separate line instead because it not closely related to the other line, and because they both benefit from their own comments.linguist-generated=trueline forgix-packetline-blocking/src/because ifgix-packetline-blocking/src/were somehow ever to gain fixture scripts--which is admittedly unlikely, since it is asrcdirectory--then it should be clear that those would not contribute to the count. The order does not actually affect that, because one is settinglinguist-vendoredtofalse, while the other is settinglinguist-generatedtotrue. But I think this order is still clearer for that.