Skip to content

Allow to ignore files during diff generation #18814

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

Closed
wants to merge 1 commit into from

Conversation

delvh
Copy link
Member

@delvh delvh commented Feb 18, 2022

Going forward, this PR allows us to ignore certain files while generating diffs.
While the functionality is currently unused, I intend to use it in a follow-up PR in the next days/ weeks to implement viewing files in a PR.
I could also postpone this PR and then post all changes at once in the other PR (once it's ready).

The "other PR" is #19007 and was implemented without this functionality.
As it is thematically isolated and decreases the size of the other PR (it will likely become large), I would like to merge this now.
Once both PRs are merged, we can hopefully see a huge performance boost for PRs with large files that have already been viewed by the user.

I currently know of one problem and of one potential problem that need to be answered before merging this PR:
1. How should the ignored files be reported? Because with the proposed approach, they will simply be missing in the output, which is understandable, but not really user-friendly. Is there an inexpensive way to get all information stored in the DiffFile struct from git?
Not a problem as I only had the use case for #19007 in mind which was now implemented without it.
2. How are the git commands executed? In particular, will the quoting ('') be problematic on Windows systems? I don't have a Windows system at hand right now, so it would be good if someone else could confirm this.

@delvh delvh added this to the 1.17.0 milestone Feb 18, 2022
@delvh delvh self-assigned this Feb 18, 2022
@singuliere
Copy link
Contributor

This is interesting 👍 Could you please add testing to verify it behaves as intended?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 18, 2022
@singuliere singuliere added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 18, 2022
@delvh
Copy link
Member Author

delvh commented Feb 18, 2022

Well... The problem with testing at the moment is pretty much that what I mentioned in 1. and 2....
I mean, I can test for its absence, but more is right now not possible.

@stale
Copy link

stale bot commented Apr 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 27, 2022
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jun 4, 2022
@delvh delvh modified the milestones: 1.17.0, 1.18.0 Jun 9, 2022
@stale stale bot removed the issue/stale label Jun 9, 2022
argsLength += len(files) + 1
argsLength += len(files)

// git 1.9.0 added pathspec exclusion of files
Copy link
Member

Choose a reason for hiding this comment

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

Now the minimal git version is 2.0, so version check is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, true.
I'd still like to add some tests before allowing this PR to be merged.
However, I'm still unsure how to do that in this case:
I know there are former git repositories in our codebase that are used for testing somehow, but I have no idea how or where to add tests.

I'll fix it when I add the tests as well.

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 26, 2022
@jolheiser jolheiser modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023
@delvh delvh removed this from the 1.20.0 milestone Jun 4, 2023
@puni9869
Copy link
Member

puni9869 commented Sep 1, 2023

@delvh are you working on it?

@delvh
Copy link
Member Author

delvh commented Sep 1, 2023

Not really

@delvh delvh closed this Sep 1, 2023
@delvh delvh deleted the ignore-files-during-diffs branch September 1, 2023 06:59
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants