From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS4713 221.184.0.0/13 X-Spam-Status: No, score=-2.7 required=3.0 tests=AWL,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=no autolearn_force=no version=3.4.2 Received: from neon.ruby-lang.org (neon.ruby-lang.org [221.186.184.75]) by dcvr.yhbt.net (Postfix) with ESMTP id 361701F4B7 for ; Wed, 21 Aug 2019 13:59:25 +0000 (UTC) Received: from neon.ruby-lang.org (localhost [IPv6:::1]) by neon.ruby-lang.org (Postfix) with ESMTP id B6A56120925; Wed, 21 Aug 2019 22:59:16 +0900 (JST) Received: from o1678948x4.outbound-mail.sendgrid.net (o1678948x4.outbound-mail.sendgrid.net [167.89.48.4]) by neon.ruby-lang.org (Postfix) with ESMTPS id 5BF9712090A for ; Wed, 21 Aug 2019 22:59:13 +0900 (JST) Received: by filter0071p3mdw1.sendgrid.net with SMTP id filter0071p3mdw1-31444-5D5D4E33-45 2019-08-21 13:59:15.848991395 +0000 UTC m=+332662.387022900 Received: from herokuapp.com (unknown [54.226.103.57]) by ismtpd0004p1iad1.sendgrid.net (SG) with ESMTP id 30W5sjCIRISresqBTvxpXQ for ; Wed, 21 Aug 2019 13:59:15.807 +0000 (UTC) Date: Wed, 21 Aug 2019 13:59:15 +0000 (UTC) From: takashikkbn@gmail.com Message-ID: References: Mime-Version: 1.0 X-Redmine-MailingListIntegration-Message-Ids: 70007 X-Redmine-Project: ruby-trunk X-Redmine-Issue-Id: 16112 X-Redmine-Issue-Author: k0kubun X-Redmine-Sender: k0kubun X-Mailer: Redmine X-Redmine-Host: bugs.ruby-lang.org X-Redmine-Site: Ruby Issue Tracking System X-Auto-Response-Suppress: All Auto-Submitted: auto-generated X-SG-EID: =?us-ascii?Q?9bN4wU0cxWeLAEOz6NELGbwCNAokUg6cnjH8nvx=2FqgbVuNRcgDVMDfXKuN+P6D?= =?us-ascii?Q?vQNlY947H=2FhY7xYJDG5oWOwMq=2FHDnJGyo=2FoFw=2FV?= =?us-ascii?Q?0RNdrkT3PXLhMyyXD5soLVn8ygJVmjsmf5nWVLs?= =?us-ascii?Q?1xEXTn2aCNr83Xik4kWcn4l4OK547R9YF+J2=2FkH?= =?us-ascii?Q?XIJN7+I1AZaDcjlUpflvAAfau1gO2+AYTYQ=3D=3D?= To: ruby-core@ruby-lang.org X-ML-Name: ruby-core X-Mail-Count: 94458 Subject: [ruby-core:94458] [Ruby master Misc#16112] Reduce the possibility of "expand tabs" commit occurrences X-BeenThere: ruby-core@ruby-lang.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: Ruby developers List-Id: Ruby developers List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ruby-core-bounces@ruby-lang.org Sender: "ruby-core" Issue #16112 has been updated by k0kubun (Takashi Kokubun). There's Option 5; altering commits in pre-receive hook on server side. But I think successful `git push` should never make future `git pull` fail. It is a very surprising behavior and would confuse committers. So I'm rejecting this option by myself. By the way, my personal position is taking either 1 or 2, and optionally implementing 3 and/or 4 as well. So I'm supporting all of option 1, 2, 3, and 4. To help the discussion, let me summarize my current understanding of pros/cons in each option: ## 0. Disable "expand tabs" in auto-style commit hook Supporters: @mame ### Pros * No noisy commits just for styling will be made, which makes reverts and backports easier. ### Cons * Since we've seen "expand tabs" commits, taking this option means to possibly start increasing the number of inconsistent indentations, effectively reopening [Bug #14246]. ## 1. Expand all tabs at once for all files managed by auto-style. Supporters: @alanwu, @byroot, @k0kubun ### Pros * It would reduce the number of problems for editors like Sublime Text, VSCode, and Atom. * More tools will be able to flawlessly show indentation of CRuby's code / patch (Compiler's preprocessed results used in MJIT, Slack text snippet, Code listing of some blog services) * All other options are very unlikely to achieve the above benefits within coming years. ### Cons * Pollute `git blame` result when `-w` option is not specified. * Tab-indented existing patches and reverts/backports will face conflicts. ## 2. Apply "expand tabs" only for newly-added lines Supporters: @k0kubun ### Pros * We're very unlikely to see "expand tabs" for new patches written with a proper editor config. ### Cons * We'll see "expand tabs" for old patches, or code written with an outdated editor config. * It slows down the progress towards consistent indentation, while it does not reverse or stop it unlike the option 0. ## 3. Prepare a local pre-commit hook to perform "expand tabs" Supporters: @k0kubun ### Pros * People using this tool will not see "expand tabs" for their own commits, and not be bothered for reverting them. ### Cons * This cannot be always applied. People not using this tool will make "expand tabs" commits. ## 4. Make pull request CI fail when there's diff to be expanded Supporters: @shyouhei, @k0kubun ### Pros * Patches merged from GitHub pull requests will not make "expand tabs" commits. ### Cons * Direct pushes to master and pull requests branched from older commits without the CI setting will make "expand tabs" commits. ---------------------------------------- Misc #16112: Reduce the possibility of "expand tabs" commit occurrences https://bugs.ruby-lang.org/issues/16112#change-80891 * Author: k0kubun (Takashi Kokubun) * Status: Open * Priority: Normal * Assignee: ---------------------------------------- ## Problem * While we agreed to use only spaces for indentation of C code in [Bug #14246], we sometimes hit "expand tabs" commit when we just edit a part of existing lines with hard tab indentation. * "expand tabs" commit bothers people when we need to perform a revert or a backport. * However, because [Bug #14246] aimed to eventually make indentations consistent, we do not want to just drop "expand tabs". * One of the motivations to solve [Bug #14246] is that having hard tabs makes preprocessed MJIT header ugly and it makes debugging on GCC hard. As MJIT may introduce C-code inlining for sources outside vm.c in the future, we want to fix the issue in almost all C sources which can be run on runtime. ## Possible Solutions There would be some options to approach the problem. I'd like to hear opinions about these options. 1. Expand all tabs at once for all files managed by auto-style. * In [Bug #14246], this was clearly objected for the reason "Indents should become consistent over time". * In my understanding, not folliwng "Indents should become consistent over time" would be problematic mainly for polluting "git blame" and conflicts on backport. * For the first point, we can use `-w` option of `git blame` to ignore that. * For backport, it's a trade-off with many "expand tabs" commits. We need opinions about this from @usa and @nagachika . * Also reverting a commit before the commit expanding all tabs would be bothering, for a short while. 2. Skip expand tabs for existing lines when indentation is not changed, and expand tabs only in newly-added lines. * If editors are configured properly, "expand tabs" would not happen for new patches in this approach. * Even in this approach, at least we will not go to the opposite direction of eventually achieving [Bug #14246]. So it seems acceptable. 3. Prepare a local pre-commit hook to perform "expand tabs", and let people who do not want "expand tabs" use it. 4. Make pull request CI fail when there's diff to be expanded, and let people who do not want "expand tabs" commit things from pull requests. ---Files-------------------------------- Screen Shot 2019-08-19 at 2.43.06 PM.png (48.8 KB) -- https://bugs.ruby-lang.org/