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

GitRepository::getCommit seems to ignore git.diffcontext #128

Open
ayaankazerouni opened this issue Mar 6, 2018 · 5 comments
Open

GitRepository::getCommit seems to ignore git.diffcontext #128

ayaankazerouni opened this issue Mar 6, 2018 · 5 comments

Comments

@ayaankazerouni
Copy link
Contributor

ayaankazerouni commented Mar 6, 2018

It sticks to the Git default of 3, even though I've set the git.diffcontext System property. The private method getSystemProperty successfully finds the value I set, but the diff output remains the same.

@ayaankazerouni
Copy link
Contributor Author

It looks like in getCommit, we get the diff for the commit using diffsForTheCommit, which does set the context based on the user-defined System property. But in getDiffText, we process the DiffEntry that we received from diffsFromTheCommit, and the DiffFormatter used here does not set the diff context.

So, after only cursory checking,

  1. We should call setContext(df2) in getDiffText. I've added this call on my local copy, and the diffs from RepoDriller now match what I see in the terminal, where the user-defined context is respected.
  2. I'm unsure why we process diffs and then process them again, which might be a broader issue.

@mauricioaniche
Copy link
Owner

It indeed seems that there's some repetition. I'll try to understand why it happens and submit a PR -- ofc, if you have time, you can submit the PR yourself!

Thanks for spotting it, btw!

@ayaankazerouni
Copy link
Contributor Author

If a fix isn't issued until around mid-April, I'm happy to work on it. But I can't until then :(

@ayaankazerouni
Copy link
Contributor Author

ayaankazerouni commented May 1, 2018

Ok, looks like in getCommit, we use diffsForTheCommit to get a List<DiffEntry>. These DiffEntry objects respect the user-defined context property, and the DiffFormatter writes to a DISABLED_OUTPUT_STREAM, so it seems no diff texts are stored. This list is only used to check the number of diffs, and to error out if the commit touches too many files.

If we pass this check, then we turn each Diff into a Modification, which eventually calls getDiffText on each DiffEntry. This uses its own DiffFormatter with a specified output stream, and this is where the user-defined context setting is overwritten.

This is my understanding of what's going on. Any insight as to why? I have the time issue a fix now, but I want to clarify the scope of this fix first (i.e., only setting the context, or refactoring to use only one DiffFormatter instance).

@ayaankazerouni
Copy link
Contributor Author

Any insight as to why?

ping @mauricioaniche

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