From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 45BEDC433ED for ; Wed, 31 Mar 2021 17:37:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E7D6F60BD3 for ; Wed, 31 Mar 2021 17:37:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233757AbhCaRgx (ORCPT ); Wed, 31 Mar 2021 13:36:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58132 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234596AbhCaRgv (ORCPT ); Wed, 31 Mar 2021 13:36:51 -0400 Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8D71BC061574 for ; Wed, 31 Mar 2021 10:36:50 -0700 (PDT) Received: by mail-ed1-x52d.google.com with SMTP id dm8so23288415edb.2 for ; Wed, 31 Mar 2021 10:36:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:user-agent:in-reply-to:date :message-id:mime-version; bh=wDTPURzC464UVXNvQ4mFHVyRGIWyarFquduGhztfPtQ=; b=Nv9kQrJcdNgeiPBcI5G7k47Bo4J16Bj6Mpc3Q0t7lpga2l4w1SDYU3ew5jqRbtOd3T x8SyaQ2w/jBcwJ0HombEbVqHH/t7AW78M1rlaQlQ07CWYHsuXNGXFSut62vbJeoYtSgz 9F+HAxWNoYrAWJ84k8ANACigRWzqSDNruv1gb1/PTNZxGH1nutAC2JEiGfmhT7PRKRuy 3us96w1wqZyoP75d/rZ1NL1b9NcLqz5yeEoYMATRrmIbs9AdPHxXoCQlAzB5or3dW+tV n4P4/6KGiJx11uDe76LT49KTtUkjs/l8DfO9xTO2OxfKhPQbTuUGPQokfOxXjKvYKHf+ gwdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:user-agent :in-reply-to:date:message-id:mime-version; bh=wDTPURzC464UVXNvQ4mFHVyRGIWyarFquduGhztfPtQ=; b=hJuefwhNXQbS+V3IGpb8DCFuCM2ZT4XLBgZBup7vpd4aTr2NQ4DIUNPkuvaZcf4s33 XWCHLgGMaiygI9KVyl7PsbTlHuhKDPTIsdDCHz5qaSLngtm/p8rdEz0ewQrANM3AFcZS qNhKgdPkoE9+KwF155P8CW3afXWJiNbvwkg4TITxT00FVDSGkzI/b8JCTywzEt+D3x7U jVVFwZ+xGF3XVBt0wwHm+GHMPuae1s89cX8uUywpoNY1a4+I1dG/fQ2h5VLP622c57KQ hyZOVmGiUHwxDooE9YuEB16SfYuTps9ebggU7i6QRqRlx+tc/v5grUO3Di7I2zJ7PxhY 4SnA== X-Gm-Message-State: AOAM532pHIBnHxy10uSW85MdAO3eO5IffgySV6X+fawMjbq6ftIZQCKN nhLRGvk+Kzs2uMPeqQa7qcQ= X-Google-Smtp-Source: ABdhPJz8FuQ9wgEmsvlKP5QBUXehd3vG71JgJRG4uOZyYG9k0fl0w82cTYQFYg2uXpUtFFzk667J4A== X-Received: by 2002:a05:6402:c8:: with SMTP id i8mr5209062edu.57.1617212209242; Wed, 31 Mar 2021 10:36:49 -0700 (PDT) Received: from evledraar (j57224.upc-j.chello.nl. [24.132.57.224]) by smtp.gmail.com with ESMTPSA id r4sm1531236ejd.125.2021.03.31.10.36.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 31 Mar 2021 10:36:48 -0700 (PDT) From: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason To: Elijah Newren Cc: Junio C Hamano , Johannes Schindelin , Elijah Newren via GitGitGadget , Git Mailing List , Philip Oakley , Phillip Wood Subject: Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages References: User-agent: Debian GNU/Linux bullseye/sid; Emacs 27.1; mu4e 1.4.15 In-reply-to: Date: Wed, 31 Mar 2021 19:36:48 +0200 Message-ID: <87mtujkyxb.fsf@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Mar 30 2021, Elijah Newren wrote: > On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano wrote: >> >> Johannes Schindelin writes: >> >> >> @@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) >> >> "--signoff", opts->signoff, >> >> "--no-commit", opts->no_commit, >> >> "-x", opts->record_origin, >> >> - "--edit", opts->edit, >> >> + "--edit", opts->edit == 1, >> > >> > Honestly, I'd prefer `> 0` here. >> >> Unless somebody (including Elijah) is trying to soon introduce yet >> another value to .edit member, I'd agree 100%. If it is a tristate >> (unspecified, no, yes), I think "is it positive" should be the way >> to ask "does the user definitely wants it?", "is it zero" should be >> the way to ask "does the user definitely declines it?" and "is it >> non-negative" (and "is it negative") the way to ask "does the user >> care (or not care)?". Using that consistently is good. > > Sounds good; I'll switch it over. > >> >> +static int should_edit(struct replay_opts *opts) { >> >> + assert(opts->edit >= -1 && opts->edit <= 1); >> > >> > Do we really want to introduce more of these useless `assert()`s? I know >> > that we stopped converting them to `BUG()`, but I really dislike >> > introducing new ones: they have very little effect, being no-ops by >> > default in most setups. >> >> Yeah, in a new code in flux where programmers can easily make >> errors, "if (...) BUG()" may not be a bad thing to add (but then we >> may want to see if we can make the codepaths involved less error >> prone), but I agree with your view that assert() is mostly useless. >> A comment that explains the expectation and why that expectation is >> there would be more useful. > > Since you both don't like this assert, I'll remove it. But I strongly > disagree that assert is useless in general. If you two have such a > strong reaction to assert statements, though, would you two prefer > that I add a new affirm() function that is ALSO compiled out in > production? Because I really want to use one of those. My operating > assumptions with asserts are the following: > > 1) If the check is relevant for production, assert() statements should > NOT be used; if(...) BUG() should be used instead. > 2) assert statements will be compiled out in production, almost always > 2a) NOTE: don't make asserts expensive, since a few production users > will keep them > 3) assert statements will be active for future me and some other folks > doing active git development > > Do you two disagree with any of those operating assumptions? I find > asserts very valuable because: > > * It's a _concise_ code comment that is readily understood. Any > attempt to word in English the same thing that an assert statement > checks always takes longer > * It helps future folks tweaking the rules to catch additional > locations where assumptions were made about the old rules. In the > development of merge-ort, for example, asserts shortened my debugging > cycles as I found and attempted new optimizations or added new > features or changed data structures and so on. The checks were _only_ > assists while developing; once the code is right, the checks could be > removed. But future development might occur, so it'd be nice to have > a way to keep the checks in the code just for those future developers > while production users remove them. > > In particular, for merge-ort, I think the second point is very > helpful. What can achieve the "remove these now-unnecessary checks > from the code for production, but keep them there for future > development"? I thought assert() was created exactly for this > purpose. Would you rather I created an affirm() that does essentially > the same thing and is compiled out unless DEVELOPER=1? That would > allow us to declare all assert() calls in the code as buggy, but I'm > not sure affirm() is as readily understood by developers reading the > code as "ooh, a reminder I get to assume these statements are true > while I'm reading the rest of the code". I don't mind the asserts, or to have them in the default build. But if you'd like to submit patches for asserts and can't otherwise get them accepted, then can we please not make DEVELOPER a thing that you can't turn on in production without thinking twice? Per my https://lore.kernel.org/git/87wnusj6gt.fsf@evledraar.gmail.com/ >> >> + if (opts->edit == -1) >> > >> > Maybe `< 0`, as we do elsewhere for "not specified"? >> >> Yup. >> >> >> + /* >> >> + * Note that we only handle the case of non-conflicted >> >> + * commits; continue_single_pick() handles the conflicted >> >> + * commits itself instead of calling this function. >> >> + */ >> >> + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0; >> > >> > Apart from the extra parentheses, that makes sense to me. >> >> I can take it either way (but personally I think this particular one >> is easier to see as written---this is subjective). >> >> > ... >> > The rest looks good, and the comments are _really_ helpful. >> >> Yup, I agree. >> >> Thanks for a review. > > Indeed; and thanks to you as well Junio for all your time reviewing.