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(pre-commit-hook): update pre-commit hook to correctly handle .diff file #18008

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

Conversation

Akshit517
Copy link

@Akshit517 Akshit517 commented Feb 23, 2025

Description

Fix pre-commit hook creating empty .diff files by removing empty .diff during ktlint formatting.

Fixes

Approach

  • Remove the .diff file when there are no unstaged changes.
  • Fixed syntax: if [apply_exit_code -ne 0] -> if [ $apply_exit_code -ne 0 ]

How Has This Been Tested?

  • Ran pre-commit hook with Kotlin files.

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@Akshit517
Copy link
Author

@Arthur-Milchior does this solve the issue ?

pre-commit Outdated
@@ -23,7 +23,7 @@ runKtlint () {

date=$(date +%y%m%d_%H%M%S)
diff_path=.git/unstaged-ktlint-git-hook-$date.diff
git diff --color=never > $diff_path
git diff HEAD --color=never > $diff_path
Copy link
Member

@BrayanDSO BrayanDSO Feb 24, 2025

Choose a reason for hiding this comment

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

this isn't in the original pre-commit hook. Is it necessary? If so, why?

Copy link
Author

Choose a reason for hiding this comment

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

@BrayanDSO It seems I misinterpreted the logic here. I mistakenly included both unstaged and staged changes when that wasn’t necessary.

The hook was actually designed to save unstaged changes in a .diff file, apply the linter to staged changes, and then reapply the unstaged changes afterward.

The issue was that when there were no unstaged changes, it was still creating an empty .diff file.

I am pushing new changes that remove the empty .diff file. Apologies for the confusion!

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2025
@Akshit517 Akshit517 changed the title fix(pre-commit-hook): update pre-commit hook to correctly handle diff fix(pre-commit-hook): update pre-commit hook to correctly handle .diff file Feb 25, 2025
@Akshit517 Akshit517 requested a review from BrayanDSO February 25, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-commit hook don't log diff
2 participants