mauricioaniche/repodriller

GitRepository::getCommit seems to ignore git.diffcontext

Opened this issue · 5 comments

ayaankazerouni commented

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 commented

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 commented

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!

  • 👍1
ayaankazerouni commented

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

ayaankazerouni commented

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 commented

Any insight as to why?

ping @mauricioaniche