Draw ruler and box around headers

[ericbn]

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. E.g. 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.

[ericbn]

This is how it looks like:

Before / After

[ericbn]

horizontal_rule was already printing $reset_color at the end of the line, so an extra $reset_color here was redundant.

[scottchiefbaker]

Several of the things you fixed are already fixed on next. All PR should be based against that branch. Can you redo this PR to target the correct branch.

[ericbn]

Hi @scottchiefbaker, sure! Let me rebase it.

[ericbn]

@scottchiefbaker, I apologize about the confusion I've made around the fix for the "Use of uninitialized value $less_charset in pattern match" error. I didn't notice it was already fixed in the next branch. I've just removed the fix that I was suggesting for this same error in this PR.

The only failure still left in this PR is in the macOS build, which seems to have been fixed in #403, but was merged to master instead of to next.

[scottchiefbaker]

I've been hemming and hawing over this issue for a while. Personally I don't think I would use, but I can definitely some people in the community wanting it. The issue then becomes that we need to make this a configuration option to allow people to opt in/out of this feature.

Right now every time d-s-f starts up we shell out to git config --list for every config item. This works, but it's slow, and it only get slower as we add more options. I need to ponder how best we can add this feature without slowing down the startup of d-s-f. It's already slower than I would like it.

[ericbn]

Cool. I see d-s-f is already caching the git config output in $static_config. But it parses it once for each call of sub git_config. So maybe the parsed result can be cached instead, to avoid that extra redundant parsing.

Besides that, the code also uses DiffHighlight::color_config, and that can be replaced by respective calls to sub git_config, which takes advantage of the cached config. DiffHighlight::color_config on the other hand, will call git config each time it's called, as you mentioned.

Also, I'm doing the following in the only config-related code that I've added:

		if ($line =~ /^${ansi_color_regex}commit [0-9a-f]{40}/) {
			$commit_color = $1 || get_config_color("commit");
			print_commit_box($line);

meaning it's first tries to use the color matched in the regexp, using the same optimization that you already pointed out before.

So there's room to make it even more optimized than what it was before this PR. :- )

Are you benchmarking d-s-f with any specific script?

[scottchiefbaker]

@ericbn I just landed some commits to handle caching of git_config() that might speed things up a bit. They're on next take a look and we can build from there.

[scottchiefbaker]

@ericbn I updated #278 with some more details. If we can get that figured out, landing this will be a no-brainer.

[scottchiefbaker]

@ericbn I'm circling back to this now that I got our startup issues figured out. I removed the Encode module all together because it was slowing down startup. I see you reference it here... I think all of those encode calls could be hardcoded. That's what I did with the horizontal rule character.

[ericbn]

Hi @scottchiefbaker. I'm loving all the recent optimizations. I've just rebased the PR commit and updated it to hardcode the UTF-8 encodings. Thanks!

[scottchiefbaker]

Does this kick in if there is a commit line, or all the time? Seems like this is a git show thing, not a git diff?

[ericbn]

@scottchiefbaker, correct: the "commit lines" show up in git show and git log, but not in git diff. The git diff only has the "meta lines", for which I'm changing the formatting to a tighter box that goes exactly around the text width. And all the formatting is the same regardless of the output that is being shown.

Do you want me to change it so the "top hierarchical lines" (commits in show and log, meta in diff) will always show with the wide lines, and the "bottom hierarchical lines" (meta in show and log, N/A in diff) will always show with the tighter box?

[ericbn]

@scottchiefbaker, I've updated the PR so the behavior with git diff is unchanged (only ruler is drawn around each meta line, like in the screenshot in the README.md). For git show and git log it will print the ruler around commit lines, and a box around meta lines, to distinguish between the different header levels (like in the screenshot posted above).

[ericbn]

Hi @scottchiefbaker. Anything I can still do in this PR for you to reconsider it? I'm refactoring some of the current code here, and at the end the change only has 10 extra lines (and 5 of those are comment lines).

[scottchiefbaker]

@ericbn thanks for bringing this back up... It slipped off my radar.

Ultimately I think it boils down to "do our users want this". It adds two additional lines to each commit line, and changes how the added/modified/removed lines work. Anytime there is change, users notice (and complain). I think it boils down to "is this a good enough addition to warrant the change", and I don't have a good answer to that.

Perhaps we should take a vote of the core developers to see what their take is?

[ericbn]

Hi @scottchiefbaker.

Sure, votes from the core developers sounds good to me. Please all note that the changes I'm proposing here don't affect the output of git diff in any way. The output only changes for git show and git log (and any other git command that shows one or more commit lines separating commits being listed).

EDIT: Hopefully this is the kind of change the users will notice and appreciate! :- )

[vraravam]

I for one would definitely vote for this feature enhancement!

[Svalorzen]

Any news about this change? This PR seems to have been here a while. I also support making more visible the distinction between different commits; after all, if one is trying to show multiple commits rather than one large patch it means that looking at each commit's diff individually is valuable, and so the start and end point of each commit should be more visible (certainly more visible than the header for each individual file change).

[OJFord]

@scottchiefbaker wrote:

Personally I don't think I would use, but I can definitely some people in the community wanting it. The issue then becomes that we need to make this a configuration option to allow people to opt in/out of this feature.

and later:

Anytime there is change, users notice (and complain). I think it boils down to "is this a good enough addition to warrant the change", and I don't have a good answer to that.

Perhaps we should take a vote of the core developers to see what their take is?

personally I do like it, but given a config option it could just be default off if concerned about that? Leave default-on for a major version bump?

[scottchiefbaker]

I'm going to close this issue and move discussion over to #497.