Project

General

Profile

Actions

Misc #18891

closed

Expand tabs in C code

Added by k0kubun (Takashi Kokubun) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
[ruby-core:109115]

Description

Problem

There's no way to handle tab/space-mixed indentation of CRuby's C code properly in VSCode. It impacts the productivity of the VSCode users a lot, including myself.

Suggested solutions

I implemented option 2 out of the following options at https://github.com/ruby/ruby/pull/6094.

  1. You tell me how to do that in VSCode.
  2. Expand all tabs in most of the C code at once and add the commit to .git-blame-ignore-revs
    • pros: GitHub and git blame --ignore-revs-file support it. Unlike option 3, commit logs and .git-blame-ignore-revs can be short.
    • cons: It might be a bit difficult to find out which C files shouldn't be a target. Bare git blame could be inconvenient.
  3. Allow expanding all tabs in each C file as we want and manage the list of such commits in .git-blame-ignore-revs
    • pros: GitHub and git blame --ignore-revs-file support it. You don't introduce unnecessary changes to files you don't care about.
    • cons: We might see too many commits for this in the commit history. Bare git blame could be inconvenient for such files.
  4. Discourage contributions to CRuby from VSCode users. (proposed by @graywolf (Gray Wolf))

Drawbacks

The expected drawbacks of doing option 2 are:

  • Existing patches on hard-tab lines may conflict.
  • Backporting patches on hard-tab lines made after this commit to revisions before this commit may conflict.

I assume that resolving such conflicts wouldn't be too difficult and this problem will not last forever.

Out of scope

I'm not interested in discussing the adoption of clang-format. It would change a lot of things that are not related to the problem.

Past discussion

https://bugs.ruby-lang.org/issues/16112 touched this topic before, but we haven't discussed --ignore-revs-file so much. I also didn't know VSCode still can't deal with it in 2022 (as I've been using Vim).

Updated by graywolf (Gray Wolf) over 1 year ago

One more solution would be:

  1. Do not use broken editors.

¯\_(ツ)_/¯

Should be at least listed here as an option.

Updated by k0kubun (Takashi Kokubun) over 1 year ago

  • Description updated (diff)

Sure, I added your solution to the description.

Updated by Eregon (Benoit Daloze) over 1 year ago

Thank you for the clear and precise issue.

(2) seems very obviously the way, and it's amazing it has not been done yet.
This is the #1 problem for new (and some of the existing as well) contributors to CRuby..
If CRuby does not do this, it is explicitly rejecting new contributors for no good reasons.

(IMHO mixing spaces and tabs has no sense nowadays, it's an antique legacy non-sense practice with zero advantages, and there is no valid reason to keep that monster alive)

Updated by k0kubun (Takashi Kokubun) over 1 year ago

It might be a bit difficult to find out which C files shouldn't be a target.

I noticed ruby-commit-hook/bin/auto-style.rb already knows about this. (2) doesn't have any blocker.

So I prepared a pull request: https://github.com/ruby/ruby/pull/6094. I confirmed GitHub ignores it just fine. If you have git config --global blame.ignoreRevsFile .git-blame-ignore-revs git config blame.ignoreRevsFile .git-blame-ignore-revs, git blame ignores it properly too. I'm inclined to move forward with this unless somebody has a problem with leaving such a commit.

Actions #5

Updated by k0kubun (Takashi Kokubun) over 1 year ago

  • Description updated (diff)
Actions #6

Updated by k0kubun (Takashi Kokubun) over 1 year ago

  • Description updated (diff)

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

I'm also in favour of https://github.com/ruby/ruby/pull/6094 - It feels like the most elegant solution of those proposed

Updated by nagachika (Tomoyuki Chikanaga) over 1 year ago

I also support the 2nd solution from the perspective of stable branch maintenance.

Updated by mame (Yusuke Endoh) over 1 year ago

@nagachika (Tomoyuki Chikanaga) Thank you for your comment. I vote for 2 too.

If CRuby does not do this, it is explicitly rejecting new contributors for no good reasons.

I want to use VSCode, but this expression is not correct in my perspective. VSCode is explicitly rejecting new users who have been working on any project with emacs-style indentation.

Updated by mame (Yusuke Endoh) over 1 year ago

  • Status changed from Open to Assigned

We discussed this issue at the dev meeting.

Finally, we were able to reach an agreement to expand hard tabs. Congratulations!

Updated by etienne (Étienne Barrié) over 1 year ago

If you have git config --global blame.ignoreRevsFile .git-blame-ignore-revs, git blame ignores it properly too.

Unfortunately you can't set it at the global level because git blame fails if the file does not exist, and that would cause issues when working on other repos (see this discussion in the Git mailing list). But you can definitely add it as a config local to the repo, and mention how to do that at the top of the file itself.

I would also recommend setting these two configs which can help in case the ignored revisions end up hiding the origin of a line: blame.markUnblamableLines and blame.markIgnoredLines. Looking at the output of git blame at least you'll see that a revision was ignored. These two can be set at the global level.

Updated by k0kubun (Takashi Kokubun) over 1 year ago

Unfortunately you can't set it at the global level because git blame fails if the file does not exist, and that would cause issues when working on other repos

Right, I noticed that behavior after writing the comment but forgot to update the comment about it. I corrected the comment to not use --global, which would write .git/config. Given that you could always use ~/.gitconfig or .git/config for your original git configuration, I guess it's fair to just commit .gitconfig to the repository. I'll add that once I merge it. (edit: I noticed committing .gitconfig doesn't work and you'd need to do something in .git/config like including it anyway) I'll leave a comment about it in .git-blame-ignore-revs. Hopefully, you wouldn't need to initialize your repository so often.

I would also recommend setting these two configs which can help in case the ignored revisions end up hiding the origin of a line: blame.markUnblamableLines and blame.markIgnoredLines.

Good to know. I guess the change of this time doesn't introduce new lines, but I'll consider adding that when I see something weird that can be solved by that.

Actions #14

Updated by k0kubun (Takashi Kokubun) over 1 year ago

  • Status changed from Assigned to Closed

Applied in changeset git|5b21e94bebed90180d8ff63dad03b8b948361089.


Expand tabs [ci skip]

[Misc #18891]

Updated by k0kubun (Takashi Kokubun) over 1 year ago

I forgot to mention that I appreciate everybody who supported this decision. Thank you so much.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0