On Fri, Dec 03 2021, Elijah Newren wrote:
> On Fri, Dec 3, 2021 at 10:37 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>>
>> On Wed, Dec 01 2021, Elijah Newren wrote:
>>
>> > On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@xxxxxxxxxxx> wrote:
>> >>
>> >> > Please don't, at least not this version. There have been newer
>> >> > submissions with three commits.
>> >> >
>> >> > I also still find the word 'die' confusing, since to me it suggests
>> >> > aborting the whole am operation, and the documentation does not dispel
>> >> > that concern. Even if you don't like 'ask' (for consistency with
>> >> > git-rebase), I think 'stop' or 'interrupt' would be much better
>> >> > options than 'die'. If you really want it to be 'die', I think the
>> >> > behavior needs to be explained in the documentation, rather than just
>> >> > assumed that users will understand it (because I didn't understand it
>> >> > until I read the code).
>> >>
>> >> Dears Newren,
>> >>
>> >> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
>> >> because they still indicate that it will stop or interrupt the whole am session,
>> >> rather than stop in the middle of it.
>> >
>> > Since you've been through several rounds of revisions already, if this
>> > is the only remaining issue with your series, I wouldn't try to hold
>> > it up for this issue; I just thought it could be fixed while you were
>> > working on the --allow-empty stuff.
>>
>> FWIW I think it's worth getting the UX issue right, tweaking it is
>> relatively easy, and if we can decide on what the thing is called
>> then...
>
> :-)
>
>> > However, while I don't think it's worth holding up your series for
>> > just this issue, I would definitely submit a follow-up RFC patch to
>> > fix the wording, because I do disagree with your assertion here pretty
>> > strongly. Let's look at the meanings of the terms:
>> >
>> > die: connotes something pretty final and irreversible -- people tend
>> > not to revive after death as far as recorded history goes; most such
>> > recorded instances tend to be causes for people to debate the
>> > definition of 'dead'.
>> >
>> > stop: could be final, but is often used in a transitory setting: "stop
>> > and go traffic", "stopped to catch my breath", "the train will stop
>> > at this station", "stop! I want to get out", etc.
>> >
>> > interrupt: seems to nearly always be used as a transitory thing
>> >
>> > Now, in the computer science context, all three terms come up relative
>> > to processes. You can interrupt a process (the kernel does all the
>> > time), but it'll usually continue afterwards. Or you can give it a
>> > SIGINT (interrupt from keyboard signal), which the process can catch
>> > and ignore. You can stop a process (and SIGSTOP cannot be caught),
>> > but you can also continue it later. die essentially means the process
>> > is going to be gone for good (at least short of some kind of black
>> > magic like a reversible debugger such as rr).
>> >
>> > So, I think it's much more likely that 'die' will be misunderstood to
>> > mean abortion of the entire am-process, than that 'stop' or
>> > 'interrupt' would.
>>
>> Why are we exposing an --empty=die at all? It's what we do by default,
>> so why have it? The user can just not provide the "--empty" option, then
>> they'll get the current behavior of die_user_resolve(), which seems to
>> have inpired the name for this "die" (it exits with code 128, just like
>> die()).
>
> That's an interesting angle to take; I hadn't thought of that. It's
> worth considering.
>
> We do often introduce options equivalent to the default as a way to
> either countermand an earlier option (e.g. --do-walk overrides
> --no-walk in git log), or because we want to allow new config options
> that change the default while allowing the user to explicitly request
> something different (e.g. --no-renames was introduced at the same time
> as diff.renames), or because we may want to change the default
> behavior and want users to be able to explicitly request a certain
> type of behavior (e.g. rename detection is the default and
> --no-renames overrides).
>
> It's not clear to me whether that type of flexibility is needed or
> whether we can just leave it unnamed. Three points that may affect
> that decision: (a) the default (and actually, hardcoded) behavior
> before this series for git-am was 'drop', (b) the default behavior
> for git-rebase is 'drop' (though it only affects commits which become
> empty, something we can't determine in the context of am) and (c)
> there was one point during the series that the author asked about just
> removing the 'die' implementation and picking a different default.
>
> The above three points suggest to me that there might reasonably be
> config added to control this or that the default could change, and
> thus that it might be useful to name the interrupt-the-operation
> behavior so that users can explicitly request it. But that's
> somewhere around three levels of chained "might" conditions, so...
> :shrug:
>
>> Once we get rid of "die" the rest of the UI can follow the example of
>> the existing "git rebase" options:
>>
>> --empty={drop,keep,ask}
>>
>> I.e. the "drop" and "keep" will be the same, no "ask" currently, but it
>> can be implemented in the future.
>
> Um, there are minor contextual differences, but what rebase calls
> "ask" (interrupt the operation and tell the users what commands they
> can use to keep or drop the commit and then resume the operation) _is_
> implemented by this series -- it's just being called "die" here.
>
> That's the kind of maddening inconsistency in Git that users complain
> about that I really think we should avoid adding to. If for some
> reason 'ask' from rebase seems like a bad choice, then I think we
> should pick a new name for this interrupt-the-operation behavior that
> makes sense (unlike 'die') for git-am and add it to git-rebase as a
> preferred synonym to 'ask'.
>
>> Maybe I'm missing something, I haven't used "am" in this way (or rebase
>> with --empty=*), but this just seems to me to be needlessly exposing a
>> "die" (or "stop" or whatever) because it's how we implement this.
>>
>> Whereas for the UX we don't need to call it anything except the absence
>> of an --empty option, or perhaps --no-empty.
>
> `--no-empty` would semantically be read by users to mean get rid of
> empty commits, which would be a synonym of 'drop'. I think it'd be as
> confusing as 'die' (and maybe even more so) for naming the
> interrupt-the-operation behavior.
Ah, I didn't look into the finer details. Yes if it does maps to "ask"
in rebase we could just use that, so would this be consistent?:
--empty=die -> --empty=ask
--empty=drop -> (ditto, no change)
--empty=keep -> (ditto, no change)
I think "ask" is a bit of a weird term for this, but I think consistency
trumps a while-we're-at-it fix here.
Whatever new synonym we'd come up with (if that would be justified, that
itself would add to confusion) could be done as a follow-up and
implemented for both "rebase" and "am".
