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

Applying patch does not work for big commit. #5

Open
asefnoor opened this issue Jan 4, 2015 · 10 comments
Open

Applying patch does not work for big commit. #5

asefnoor opened this issue Jan 4, 2015 · 10 comments

Comments

@asefnoor
Copy link

asefnoor commented Jan 4, 2015

I tried to commit my code locally and it detected violations in different files. It suggested me to run git apply /tmp/pre-commit-uncrustify-1420355533.patch this command to apply patch. this command works fine and few of files are changed but when I try to commit again it gives me same violations which I observed on first run.

Please suggest.

@ddddavidmartin
Copy link
Owner

Hmmm, interesting. I presume you can not share your actual code to reproduce the issue?

Let's see whether it does what we would expect it to do:

  1. You try to commit changes and it is the right files that you changed that are being checked? It shows up similar to
Running hook: pre-commit-uncrustify
Parsing: test.c as language C
...
  1. Then, when you apply the patch with git apply, what does actually change? Does git print anything helpful? Try calling it with git apply --verbose to get some more detailed output and check whether it applies it to the correct files. Regardless, after applying the patch the changes should be applied but not yet staged for the next commit. Which means, a
git diff --cached

shows your original commit which violates the uncrustify rules and

git diff

shows the patched changes to make it comply with the rules.

  1. You then have to stage the patched changes with git add -p for example and commit again and it should be fine.

Please let me know where it seems to break and we can have a closer look.

@ddddavidmartin
Copy link
Owner

Ah, and could you let us know which OS you are using and which versions of git and uncrustify you are running? Basically what git --version and uncrustify --version tell you. Thanks!

@asefnoor
Copy link
Author

asefnoor commented Jan 5, 2015

David,

Thank you so much for prompt response and writing detailed instructions.

I have following setup
git version 1.9.3 (Apple Git-50)
uncrustify 0.60
OS X 10.9.4
When I tried to commit it showed parsing different source files

Parsing: Project/Application/App Delegate/AppDelegate.h as language OC
--------------List of files goes on
Then one by one it displayed file names and code that was not according to rules specified

The following differences were found between the code to commit and the uncrustify rules:

--- "a/Vente/Common/ASValueTrackingSlider/ASValuePopUpView.m" 2015-01-05 19:57:07.000000000 +0500
+++ "b/Project/Common/ASValueTrackingSlider/ASValuePopUpView.m" 2015-01-05 20:01:17.000000000 +0500

Then at the end

You can apply these changes with:
git apply /tmp/pre-commit-uncrustify-1420470077.patch
(may need to be called from the root directory of your repository)
Aborting commit. Apply changes and commit again or skip checking with --no-verify (not recommended).

I ran git apply /tmp/pre-commit-uncrustify-1420470077.patch on my machine and it changed some files
Then i ran git add . command and tried to commit locally. It again displayed same violations.

Thanks in Advance.
Asif

@ddddavidmartin
Copy link
Owner

You are most welcome, Asif. At this point I'm just guessing though as I have no idea yet what the problem is. Maybe you are hitting some corner case that no one else had yet. Some things coming to my mind:

  1. How many files are you committing? How big is the patch file or how many lines does it have?
  2. Try committing just a single file and see whether it works then. At least then we know a bit more about whether it is a problem in general or more specific.

@asefnoor
Copy link
Author

asefnoor commented Jan 7, 2015

David, I think you are right about big patch and impressed with your finding because I have too many files around 60 that I wanted to commit. Some of my own code and some of the third party libraries files, that is why patch became so big.
For the time being, i removed pre-commit and canonicalized-file.sh from hooks folder and committed my code to git. Now I have put back above two files back in hooks after doing commit.
I will proceed with new code and will try to commit with fewer files. If it reports violations, I will try to apply patch and will share my results on same thread.

Thanks,

@asefnoor
Copy link
Author

David,

I tried with fewer files and it worked. You were right about big patch.

Thanks man.

@ddddavidmartin
Copy link
Owner

Hi Asif,
I'm surprised that it already failed at about 60 files. I would have thought the script would be robust enough against that. I was thinking more along the lines of 10,000 files or so.

Would you be able to tell me how big the patch file was that failed? As in how many lines did it have? I may try and reproduce the issue locally and see what we can do about it. Because it would be good if the scripts just worked regardless or at least exited gracefully with a helpful error message.

Some general comments:

  • I would not uncrustify third party sources. For example if you would want to merge in a patch from upstream it could become a nightmare as the formatting would be different. Or trying to get a meaningful diff of your third party sources against a different version.
  • If you want to start using uncrustify for an existing project, it might make sense to uncrustify everything once, do one big cosmetic commit and then just make sure that all new changes are according to the rules. Without having tried it, find might be a good start. Something like find . -type f -name "*.m" -exec uncrustify -c uncrustify.conf --no-backup

Thanks for your feedback!

@asefnoor
Copy link
Author

David ,

Following are the stats in that commit

140 files changed, 3409 insertions(+), 11040 deletions(-)
Let me know if you want any more details.

Thanks,

@ddddavidmartin ddddavidmartin changed the title Applying patch does not work. Applying patch does not work for big commit. Jan 14, 2015
@ddddavidmartin
Copy link
Owner

I will see when I get some time to have a look into it. If I need any more details I will be in touch. Thanks very much for the help so far, Asif!

@asefnoor
Copy link
Author

You are most welcome. I am thankful to you for your prompt responses and
looking into issue that I was facing.

On Wednesday, January 14, 2015, David Martin notifications@github.com
wrote:

I will see when I get some time to have a look into it. If I need any more
details I will be in touch. Thanks very much for the help so far, Asif!


Reply to this email directly or view it on GitHub
#5 (comment)
.

Sent from iPhone. Please excuse any typos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants