Skip to content

Conversation

Svalorzen
Copy link

@Svalorzen Svalorzen commented Oct 18, 2024

This PR is the same as PR #398, I have just rebased it onto the current next so that there are no conflict. Just some re-tabbings were needed.

I'm doing this as the PR still seems to work well, and it would be a shame to lose the amount of work that @ericbn did.

Rulers are drawn around the first level of headers, and boxes around the second level of headers, if any. The `diff-so-fancy.rulerWidth` config still applies, but only to the rulers. Boxes have just the size of the text inside them. `git show` and `git log` have two levels of headers: (1) commit, and (2) meta (e.g. `modified: diff-so-fancy`). `git diff` only has one level of headers: meta. This allows for a better hierarchical view of commits specifically, with commit lines having a configurable wider ruler around them and meta lines ("children" of commits) having a shorter box around them.
@scottchiefbaker
Copy link
Contributor

Couple of quick questions while I review this:

  1. Can you provide a screenshot of what the output before/after this change?
  2. Does this pass all the current unit tests?
  3. This is a pretty large change so this feature would need to be behind git config option so users can opt-in to it
@OJFord
Copy link
Member

OJFord commented Oct 19, 2024

@scottchiefbaker fwiw there's a comparison screenshot in the original PR: #398 (comment)

@Svalorzen
Copy link
Author

Svalorzen commented Oct 21, 2024

I have to admit I'm not really familiar with the codebase, nor I really know Perl. This is mostly a feature that I have wanted for a long time, and after seeing the older PR stay there for a while I just thought I'd give it a spin, and I didn't really do a lot of work to make it run again -- all credit goes to the original author.

AppVeyor seems to say that the build is failing, so I'm assuming some tests do fail; perhaps looking for full lines where partial lines get outputted with this patch. I think those tests did not exist when the patch was first proposed.

If there's anything I can do with my limited knowledge to help this be put in, I can try, but as I said I'm unfamiliar with both the language and the codebase so I'm limited in what I can offer.

@Svalorzen
Copy link
Author

In fact, I can already attest to a minor bug: if a file changed takes more than one line, the line wraps are incorrect.

Current:
image

With PR:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants