Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#139] [#168] [#195] Ignore build-related files #174

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

Sereja313
Copy link
Member

@Sereja313 Sereja313 commented Sep 26, 2022

Description

Problem: At the moment, we're using the ignored option for mainly 2 purposes: 1) to ignore all files in the .git folder (.git/**/*) to ignore all build-related temporary files (the default config ignores .stack-work/**/*). A more robust alternative might be to ignore all files implicitly ignored by git.

Solution: Use git ls-files to ignore all files implicitly ignored by git.

Related issue(s)

Fixes #139
Fixes #168
Fixes #195

✅ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:
  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

✓ Release Checklist

  • I updated the version number in package.yaml.
  • I updated the changelog and moved everything
    under the "Unreleased" section to a new section for this release version.
  • (After merging) I edited the auto-release.
    • Change the tag and title using the format vX.Y.Z.
    • Write a summary of all user-facing changes.
    • Deselect the "This is a pre-release" checkbox at the bottom.
  • (After merging) I updated xrefcheck-action.

@Sorokin-Anton
Copy link
Contributor

Sorokin-Anton commented Sep 26, 2022

Am I right that now we check only staged files? And what about links to unstaged ones - will we report them?
I don't like that we scan files in their current form (so unstaged changes are checked), but only staged files.
Maybe it's better to ignore only files that are explicitly ignored?

Another comment - I am not sure that xrefcheck should work only with Git repos (and before this commit we can run this in any folder)

So I think there should be CLI opt and config flag to disable "git ls-files" usage

@Sereja313
Copy link
Member Author

Sereja313 commented Sep 26, 2022

@Sorokin-Anton

Am I right that now we check only staged files?

Yes, now we only check files tracked by git.

And what about links to unstaged ones - will we report them?

Oh, indeed links to untracked files are still valid. Thank you for noticing that!

I don't like that we scan files in their current form (so unstaged changes are checked), but only staged files.
Maybe it's better to ignore only files that are explicitly ignored?

True, but before that commit we still check unstaged changes and also have to specify ignored files twice(for git and for xrefcheck). Not sure if it makes sense to check untracked files.

Another comment - I am not sure that xrefcheck should work only with Git repos (and before this commit we can run this in any folder)

Here is quote from issue comments

We really support only git repositories, since we have a mode option that affects how anchors should be rendered.

@Sorokin-Anton
Copy link
Contributor

We really support only git repositories

@dcastro what do you think about making an option to disable "git ls-files" call? For example, someone can use xrefcheck on directory which is served by FTP server. I think it's better to give not perfect reports (when talking about rendering GitHub/GitLab anchors) then don't give any

@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch from 9c644c4 to 2af2796 Compare September 27, 2022 01:53
@Sereja313
Copy link
Member Author

Looks like #168 is no longer an issue. Added test.

@Sorokin-Anton Sorokin-Anton self-requested a review September 27, 2022 12:33
Copy link
Contributor

@Sorokin-Anton Sorokin-Anton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK should we add tests on CI which checks new behavior (git ls-files`), by the way. We can make bats script that generate new repository in temp dir, if we really need this

@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch from a7141d9 to ae18d4f Compare October 3, 2022 06:42
@Sereja313
Copy link
Member Author

Added tests

.buildkite/pipeline.yml Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
src/Xrefcheck/Scan.hs Show resolved Hide resolved
src/Xrefcheck/Core.hs Outdated Show resolved Hide resolved
src/Xrefcheck/Verify.hs Outdated Show resolved Hide resolved
src/Xrefcheck/Verify.hs Outdated Show resolved Hide resolved
@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch from fe069ad to 24fff6a Compare October 13, 2022 09:02
@dcastro dcastro changed the title [#139] Ignore build-related files [#139] [#168] Ignore build-related files Oct 13, 2022
@Sereja313 Sereja313 changed the base branch from master to Sorokin-Anton/#165-test-different-local-refs October 14, 2022 18:23
@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch 3 times, most recently from 0e3a6d5 to 9fcc9ec Compare October 14, 2022 18:39
@Sereja313 Sereja313 linked an issue Oct 14, 2022 that may be closed by this pull request
@Sereja313 Sereja313 changed the title [#139] [#168] Ignore build-related files [#139] [#168] [#195] Ignore build-related files Oct 14, 2022
@Sereja313
Copy link
Member Author

Rebased on top of #194. Also #195 is no longer an issue.

@Sereja313 Sereja313 requested a review from dcastro October 14, 2022 18:45
src/Xrefcheck/Verify.hs Outdated Show resolved Hide resolved
src/Xrefcheck/Verify.hs Show resolved Hide resolved
Base automatically changed from Sorokin-Anton/#165-test-different-local-refs to master October 18, 2022 19:21
@Sereja313
Copy link
Member Author

Also fixed a bug where the --verbose option displayed all tracked files, not just scanned ones.

@Sereja313 Sereja313 requested a review from dcastro October 18, 2022 21:46
@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch from edfc3a4 to 81b8ab4 Compare October 18, 2022 22:13
@Sorokin-Anton Sorokin-Anton mentioned this pull request Oct 19, 2022
8 tasks
Problem: At the moment, we're using the ignored option for mainly 2
purposes: 1) to ignore all files in the `.git` folder (`.git/**/*`) to
ignore all build-related temporary files (the default config ignores
`.stack-work/**/*`). A more robust alternative might be to ignore all
files implicitly ignored by git.

Solution: Use `git ls-files` to ignore all files implicitly ignored by git.
@Sereja313 Sereja313 force-pushed the Sereja313/#139-ignore-build-related-files branch from 81b8ab4 to bfbe20a Compare October 21, 2022 12:13
@Sereja313 Sereja313 merged commit 2438735 into master Oct 21, 2022
@Sereja313 Sereja313 deleted the Sereja313/#139-ignore-build-related-files branch October 21, 2022 12:16
dcastro added a commit to serokell/xrefcheck-action that referenced this pull request Oct 25, 2022
Problem: As of [v0.2.2](https://github.com/serokell/xrefcheck/releases/tag/v0.2.2),
xrefcheck now requires git to be available in the PATH.

See:
  * serokell/xrefcheck#139
  * serokell/xrefcheck#174

Solution: add `git` as a dependency in the dockerfile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants