From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,RDNS_NONE,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.4 Received: from authenticated user by zero.zsh.org with local id 1kihMh-000HfQ-F4; Fri, 27 Nov 2020 17:16:59 +0000 Authentication-Results: zsh.org; iprev=pass (out3-smtp.messagingengine.com) smtp.remote-ip=66.111.4.27; dkim=pass header.d=daniel.shahaf.name header.s=fm2 header.a=rsa-sha256; dkim=pass header.d=messagingengine.com header.s=fm1 header.a=rsa-sha256; dmarc=none header.from=daniel.shahaf.name; arc=none Received: from out3-smtp.messagingengine.com ([66.111.4.27]:46779) by zero.zsh.org with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) id 1kihMP-000HQn-Qr; Fri, 27 Nov 2020 17:16:42 +0000 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 12EF85C01A8; Fri, 27 Nov 2020 12:16:41 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Fri, 27 Nov 2020 12:16:41 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= daniel.shahaf.name; h=date:from:to:subject:message-id:references :mime-version:content-type:content-transfer-encoding :in-reply-to; s=fm2; bh=RZsiBHW1uD525Bnka6G8i46lcrjPhC6refC1Ucgj IAg=; b=bKEb8g/JBFTNSmasBl1j9cSyiGri7hk1ORbj+FSB0vqAmfiMyorEajEC Bsd9MI7crHJK2e4uCQxhI26lF0CKAchsLjrB/cyYdPigswFo0PEiTCYa0WyhZi7y EJQlrZypzixT+rHw//UJ3YNWPdUoFeXhXojyWPhpZH3u7NpmhtfE87myqxjaMR1o Zoxwzf7MQG41Bu3J+lGSO+CP+4KhleLPPbZ3cbeMrdAe8m2gxRMAdvlilaeziA+N 6TR8VMR9vxPWLWqPSLDoRKo23mxCKVp9AjwoSUK2RzCQm7m/+O5p4XuSYopcxKPv R36UqqjB0O6dYkiDw2mj7COj5Wn9qg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=RZsiBHW1uD525Bnka6G8i46lcrjPhC6refC1UcgjI Ag=; b=W5Z2/RcXZ6ItwKnxkQftk5ImP6g3w5uznwvgl/WlGoKsCXkCRVKag+6lU GpRZEiZ1gr+X4PINoKhbRb8vu525aas74QsClW0n17aZgGNiyi/riK3ynEf24Zb1 6bJCQ7ZCB7pT3DkmsiRmFCSwBf3Hd8wVA95b9mcVd/mLlGXNAfRZO6cb7mhe/Y0t r5Y0iyHcmr1guk/QdKKtBE2KgWfC1zSRA//kZhQBVAY0431J/vO8NdLqH4y/VAyK MFnYLyvu+1nr5PZQLIzm7qMZjMh2F29rP15B9BInIDx2XTTbDTiZ8g0D7Mda5rOK Ae7Gp4fPt/1maH1fjYf0I6giLBUDw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudehgedguddttdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecunecujfgurhepfffhvffukfhfgggtugfgjggfse htkedttddtreejnecuhfhrohhmpeffrghnihgvlhcuufhhrghhrghfuceougdrshesuggr nhhivghlrdhshhgrhhgrfhdrnhgrmhgvqeenucggtffrrghtthgvrhhnpefgkefgfeejgf dvvdfguddtleelkedvfeetiedtudfhveevveduhfdvveeffedvueenucfkphepjeelrddu kedtrdeikedrudefheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrih hlfhhrohhmpegurdhssegurghnihgvlhdrshhhrghhrghfrdhnrghmvg X-ME-Proxy: Received: from tarpaulin.shahaf.local2 (bzq-79-180-68-135.red.bezeqint.net [79.180.68.135]) by mail.messagingengine.com (Postfix) with ESMTPA id 69D0E3064AB3 for ; Fri, 27 Nov 2020 12:16:40 -0500 (EST) Received: by tarpaulin.shahaf.local2 (Postfix, from userid 1005) id 4CjLrk25Vvz4V4; Fri, 27 Nov 2020 17:16:38 +0000 (UTC) Date: Fri, 27 Nov 2020 17:16:38 +0000 From: Daniel Shahaf To: Zsh hackers list Subject: Re: [PATCHv1] [long] improvements to limit/ulimit API and doc Message-ID: <20201127171638.GE26720@tarpaulin.shahaf.local2> References: <20201123214942.hi2rx7n3jk25ucmd@chazelas.org> <20201125003512.GC17978@tarpaulin.shahaf.local2> <20201125064443.a6zdrwka2ajkgb2t@chazelas.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201125064443.a6zdrwka2ajkgb2t@chazelas.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-Seq: 47666 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: List-Subscribe: List-Unsubscribe: List-Post: List-Owner: List-Archive: Archived-At: Stephane Chazelas wrote on Wed, Nov 25, 2020 at 06:44:43 +0000: > 2020-11-25 00:35:12 +0000, Daniel Shahaf: > [...] > > > A few issues addressed: > > > > > > > This patch is over 1k lines long and makes multiple independent changes. > > > > Could you please split this into one patch per logical change? That > > would make it easier to review, both now, and in the future should > > a regression be bisected to it. > [...] > > Hi Daniel, > > thanks for having looking into it. > > They're not independent, except maybe for the "limit"/"unlimit" > now back in csh which is a 5 code line change. > *nod* > Chunks of the code and doc have been rewritten to address > those. Separating out the changes would mean rewriting the code > several times which would be more effort for me and reviewers, > mostly wasted if we're only keeping the last iteration. How so? In v2 2/2's log message I see, for starters, several bullet points about documentation-only bugs, and in the .c diff the showlimitvalue() stands out as _prima facie_ independent of the rest. I don't see how _splitting_ those involves a _rewrite_. Splitting is fairly easy,¹ and I don't think it'd be a "waste" of time. On the contrary, I think it'd be a O(1) effort for O(N) gain (split it once and then, even years from now, anyone who reviews the history will have a easier time). Moreover, smaller diff can help ensure that each change is tested and help catch unintentional behaviour changes (always a risk in refactorings). I do agree that a single logical change that happens to fix a dozen interdependent bugs needn't be split, but I don't see how that's the case here. Enlighten me ☺ > But maybe before looking in detail at the code, we can discuss > whether the change in API makes sense. Seems to make sense, going by the log message. Really, the only part that jumped out at me was changing the behaviour of "KB" on input from error to "1000 bytes". Cheers, Daniel ¹ Stephane: I elaborated offlist recently on splitting.