Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages

> On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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".

>> >> +    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.