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

IDA support for IDA 7.5 #123

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

IDA support for IDA 7.5 #123

wants to merge 1 commit into from

Conversation

0xeb
Copy link

@0xeb 0xeb commented Mar 19, 2021

No description provided.

@Mattiwatti
Copy link
Member

Thanks for the PR! IDA 7 support is something that has been requested quite a few times over the years, so I appreciate you taking the time to work on this.

Unfortunately I can't merge this as is, due to the fact that you have made many changes to whitespace, indentation, comments and general style, that all combined touch most of the files in the entire repository in one way or another. I don't necessarily have a problem with these changes (provided you can give arguments for why they should be merged upstream), but they should at the very least be part of a separate commit in a separate PR. But honestly I'd prefer to simply not have these changes at all - I don't really see the added benefit if you keep in mind that tools such as git blame will forever point at this one commit, even though nothing functional was changed.

Can you amend your commit (using force push) so that the changes remain confined to ScyllaHideIDAProPlugin/ and ScyllaHideIDAServer/ only?

I do have some other comments, mainly re: the project configuration for 32 and 64 bit targets, but they are comparatively minor. I'll add them in a Github review or make the changes myself once you've fixed this.

@0xeb
Copy link
Author

0xeb commented Apr 10, 2021

You are completely right in your arguments.
The code was hard to work with. I had to read it and clean it up first.

Then I squeezed in IDA 7.5+ support.

I understand if you don't merge.

@Mattiwatti
Copy link
Member

Mattiwatti commented Apr 15, 2021

Your code is fine and I'd like to merge it, just not with the additional changes to the other components unrelated to the IDA plugin.

One way to do this:

  1. Make a temporary copy of the new ScyllaHideIDAProPlugin and ScyllaHideIDAServer directories somewhere. These contain the relevant changes for the PR.
  2. git reset --hard 2276f14 to revert to the current master commit. NB: this will destroy all of your local changes, so make a backup if you want to keep them.
  3. Copy back the directories from step 1.
  4. Add the modified files to the index and commit.
  5. git push --force origin master (assuming your fork is named origin).

There's probably a smarter way to do this, but I'm not a git wizard and this will work fine. The important part is that there must only be one commit (otherwise the issues mentioned in my first comment will still exist), and the push must be a force push (otherwise it will be rejected).

Please put a comment here when you've done this, since Github doesn't send email notifications for force pushes in PRs.

@kotori2 kotori2 mentioned this pull request Sep 9, 2022
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

Successfully merging this pull request may close these issues.

2 participants