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

fix: Make Git conflict markers check more precise #6379

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yermulnik
Copy link
Contributor

@yermulnik yermulnik commented Nov 21, 2024

The #6113 introduced new Git merge conflicts linter check, that is error prone in some conditions (see screenshot).

This PR adjusts this check to be more precise and exact in a way that all three markers should be found to identify check as failed. Additionally improve Git merge conflict markers matchers.

Repro:
image

> grep -E '^(<<<<<<<|=======|>>>>>>>)' README.md
==========================================================
=============   MegaLinter, by OX.security   =============
=========  https://ox.security?ref=megalinter  ===========
==========================================================

Replaces #6374

Readiness checklist

In order to have this pull request merged, complete the following tasks.

Pull request author tasks

  • I checked that all workflows return a success.
  • I included all the needed documentation for this change.
  • I provided the necessary tests.
  • I squashed all the commits into a single commit.
  • I followed the
    Conventional Commit v1.0.0 spec.
  • I wrote the necessary upgrade instructions in the
    upgrade guide.
  • If this pull request is about and existing issue, I added the
    Fix #ISSUE_NUMBER or Close #ISSUE_NUMBER text to the description of
    the pull request.

Super-linter maintainer tasks

  • Label as breaking if this change breaks compatibility with the previous
    released version.
  • Label as either: automation, bug, documentation, enhancement,
    infrastructure.
  • Add the pull request to a milestone, eventually creating one, that matches
    with the version that release-please proposes in the
    preview-release-notes CI job.

@yermulnik
Copy link
Contributor Author

@ferrarimarco I noticed the tick was added for breaking change item in previous PR:
image
I believe this PR doesn't introduce breaking change for the previous released version as it only rectifies false positive from the previous released version (and improves overall capability of checking for Git conflict markers).

@yermulnik yermulnik force-pushed the fix/make_git_conflict_markers_check_more_precise branch from 1c5780a to dbdc0db Compare November 21, 2024 17:36
@ferrarimarco ferrarimarco added bug Something isn't working enhancement New feature or request labels Nov 21, 2024
@ferrarimarco
Copy link
Collaborator

@ferrarimarco I noticed the tick was added for breaking change item in previous PR

That's for maintainers to state that they checked the PR and deemed that as non-breaking, in this case.

The super-linter#6113 introduced new Git merge conflicts linter check, that is error prone in some conditions (see screenshot).

This PR adjusts this check to be more precise and exact in a way that all three markers should be found to identify check as failed. Additionally improve Git merge conflict markers matchers.

Repro:
![image](https://github.com/user-attachments/assets/119c9a17-652a-4a6e-88a9-108d347f6f55)
```shell
> grep -E '^(<<<<<<<|=======|>>>>>>>)' README.md
==========================================================
=============   MegaLinter, by OX.security   =============
=========  https://ox.security?ref=megalinter  ===========
==========================================================
```

Replaces super-linter#6374
@yermulnik yermulnik force-pushed the fix/make_git_conflict_markers_check_more_precise branch from dbdc0db to 8bdd4a9 Compare November 21, 2024 18:36
@yermulnik
Copy link
Contributor Author

@ferrarimarco ferrarimarco added this to the 7.2.1 milestone Nov 21, 2024
@yermulnik
Copy link
Contributor Author

Checks failure isn't relevant: super-linter/super-linter/actions/runs/11959522598/job/33342347913?pr=6379

@ferrarimarco Should I do something about this failure?

@ferrarimarco
Copy link
Collaborator

super-linter/super-linter/actions/runs/11959522598/job/33342347913?pr=6379

Not really. We just need to wait for a dependency update.

Thanks!

@yermulnik
Copy link
Contributor Author

super-linter/super-linter/actions/runs/11959522598/job/33342347913?pr=6379

Not really. We just need to wait for a dependency update.

Gotcha. Thanks. Please ping me if there's anything I need or can do about this PR.

@yermulnik
Copy link
Contributor Author

Not really. We just need to wait for a dependency update.

npm-audit: @yarnpkg/core -> @yarnpkg/shell -> Depends on vulnerable versions of cross-spawn

Looks like either of these two should bring in an update once new version of @yarnpkg/core is released:

@yermulnik
Copy link
Contributor Author

@ferrarimarco The new version of @yarnpkg/core has been released. Could you please re-run failed workflows?

@ferrarimarco
Copy link
Collaborator

@yarnpkg/core is not a direct dependency:

# npm audit report

cross-spawn  7.0.0 - 7.0.4
Severity: high
Regular Expression Denial of Service (ReDoS) in cross-spawn - https://github.com/advisories/GHSA-3xgq-45jj-v275
fix available via `npm audit fix --force`
Will install [email protected], which is a breaking change
node_modules/cross-spawn
  @yarnpkg/core  <=4.1.5
  Depends on vulnerable versions of cross-spawn
  node_modules/@yarnpkg/core
    renovate  0.0.0-semantic-release || 22.22.0 - 22.22.2 || >=22.23.0
    Depends on vulnerable versions of @yarnpkg/core
    node_modules/renovate

We'd need to wait for renovate to release a new version that either updates @yarnpkg/core, or widens the version requirements for that dependency, instead of requiring a fixed version (4.1.4 at the time of writing this).

@yermulnik
Copy link
Contributor Author

Ah, I see. Tedious =)
So I can see it's on their Dependency Dashboard already (pending status checks (whatever that means)). Wondering if it can be expedited 🤔

@echoix
Copy link

echoix commented Nov 25, 2024

I still couldn't update my repo to use 7.2.0, as it fails on patch files (diffs) we hold to apply on an external code. I don't see a way to configure that "linter". How would you suggest to handle patch files?

@yermulnik
Copy link
Contributor Author

I still couldn't update my repo to use 7.2.0, as it fails on patch files (diffs) we hold to apply on an external code. I don't see a way to configure that "linter". How would you suggest to handle patch files?

Out of interest: how your patch files content looks like?
To the best of my knowledge they don't contain merge conflict markers usually.

@echoix
Copy link

echoix commented Nov 25, 2024

I still couldn't update my repo to use 7.2.0, as it fails on patch files (diffs) we hold to apply on an external code. I don't see a way to configure that "linter". How would you suggest to handle patch files?

Out of interest: how your patch files content looks like?

To the best of my knowledge they don't contain merge conflict markers usually.

The PR: OSGeo/grass-addons#1246
The run: https://github.com/OSGeo/grass-addons/actions/runs/11895745562/job/33146047065
The affected files:

I didn't go and read how this check is implemented here, I understand that it is made in-house, but how does it compare to a similar check from pre-commit hooks? Do they have the same problem? https://github.com/pre-commit/pre-commit-hooks?tab=readme-ov-file#check-merge-conflict

@yermulnik
Copy link
Contributor Author

yermulnik commented Nov 25, 2024

Out of interest: how your patch files content looks like?
To the best of my knowledge they don't contain merge conflict markers usually.

The affected files:

As of look of it, false positives for all those files should be resolved by this PR once it's merged into super-linter and released as v7.2.1

but how does it compare to a similar check from pre-commit hooks? Do they have the same problem? pre-commit/pre-commit-hooks#check-merge-conflict

Do they have the same problem: yes and no. They use a more stricter check than what this repo has in main branch. Though still might fall error prone in some conditions as they check merge conflict markers separately (whilst these markers have to be inspected all at once — and this is what I attempted to implement with this PR).
Fingers crossed it won't take long for v7.2.1 to get released as it also should bring other fixes and improvements.

UPD: that check from pre-commit has a feature to allow transitive status of being through the merge process — I'm not sure this is worth of being implemented in super-linter at this moment as allowing such status needs to be considered by super-linter devs (and I'm not among them 🤷🏻)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants