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.

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

wants to merge 1 commit into from

Conversation

normanr
Copy link

@normanr normanr commented Oct 13, 2019

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.

Signed-off-by: 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.

Signed-off-by: Norman Rasmussen <[email protected]>
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2019

Welcome to GitGitGadget

Hi @normanr, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you want to see what email(s) would be sent for a submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

@dscho
Copy link
Member

dscho commented Oct 14, 2019

/allow

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 14, 2019

User normanr is now allowed to use GitGitGadget.

@normanr
Copy link
Author

normanr commented Oct 15, 2019

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

Preview email sent as [email protected]

@normanr
Copy link
Author

normanr commented Oct 15, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

Submitted as [email protected]

@@ -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/

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

This branch is now known as nr/diff-highlight-indent-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

This patch series was integrated into pu via git@d7a4f2b.

@gitgitgadget gitgitgadget bot added the pu label Oct 15, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 16, 2019

This patch series was integrated into pu via git@00dd658.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2019

This patch series was integrated into pu via git@1f04512.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2019

This patch series was integrated into pu via git@13b7f1d.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 22, 2019

This patch series was integrated into pu via git@67da6b1.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 22, 2019

This patch series was integrated into next via git@8ce6000.

@gitgitgadget gitgitgadget bot added the next label Oct 22, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 23, 2019

This patch series was integrated into pu via git@ef90ad4.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 23, 2019

This patch series was integrated into pu via git@b895e8d.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 23, 2019

This patch series was integrated into next via git@b895e8d.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 23, 2019

This patch series was integrated into master via git@b895e8d.

@gitgitgadget gitgitgadget bot added the master label Oct 23, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 23, 2019

Closed via b895e8d.

@gitgitgadget gitgitgadget bot closed this Oct 23, 2019
@normanr normanr deleted the diff-highlight-whitespace branch October 24, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants