Skip to content

diff-highlight: fix a whitespace nit #397

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

diff-highlight: fix a whitespace nit #397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/diff-highlight/DiffHighlight.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ sub handle_line {
(?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Oct 15, 2019 at 03:31:26AM +0000, Norman Rasmussen via GitGitGadget wrote:

> From: Norman Rasmussen <[email protected]>
> 
> This changes the indent from
>   "<tab><sp><sp><sp><sp><sp><sp><sp><sp>"
> to
>   "<tab><tab>"
> so that the statement lines up with the rest of the block.

Yep, that makes sense. Looks like I introduced the problem (most of my
perl used to be written in a style that forbids tabs, so it may have
snuck in that way, but the rest of the file definitely follows Git's
usual style of tabs).

> diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
> index 7440aa1c46..e2589922a6 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -72,7 +72,7 @@ sub handle_line {
>  	      (?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|"
>  	                         [ ]*  # trailing whitespace for merges
>  	    /x) {
> -	        my $graph_prefix = $&;
> +		my $graph_prefix = $&;

There are a few lines just above that have 8+ spaces. Arguably those
could be tabs, too, depending on your view of tabs. We usually do "8
spaces is a tab" in the Git project, but the oft-repeated "tabs to
indent, spaces to align" mantra would apply here (and I suspect you're
using a different tabwidth since you noticed this one case). So I'd just
as soon leave them be, and take your patch as-is.

-Peff

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Norman Rasmussen wrote (reply to this):

On Mon, Oct 14, 2019 at 9:20 PM Jeff King <[email protected]> wrote:
> There are a few lines just above that have 8+ spaces. Arguably those
> could be tabs, too, depending on your view of tabs. We usually do "8
> spaces is a tab" in the Git project, but the oft-repeated "tabs to
> indent, spaces to align" mantra would apply here (and I suspect you're
> using a different tabwidth since you noticed this one case). So I'd just
> as soon leave them be, and take your patch as-is.

Yep, the lines above are using the spaces to align the sections of the
multi-line if statement. This happens again for the return statements
in highlight_pair and is_pair_interesting. So this is the only line
that doesn't stick to the rule (and probably because of editor
auto-indenting). I have another change for the same change (which I'll
send once I've written tests) and I only noticed this line when I
changed my editor's default tabwidth after a while of coding.

-- 
- Norman Rasmussen
 - Email: [email protected]
 - Home page: http://norman.rasmussen.co.za/

[ ]* # trailing whitespace for merges
/x) {
my $graph_prefix = $&;
my $graph_prefix = $&;

# We must flush before setting graph indent, since the
# new commit may be indented differently from what we
Expand Down