Re: [PATCH] var: add GIT_SEQUENCE_EDITOR variable

From: Sean Allred <allred.sean@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Sean Allred via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Sean Allred <code@seanallred.com>
Subject: Re: [PATCH] var: add GIT_SEQUENCE_EDITOR variable
Date: Wed, 23 Nov 2022 06:21:47 -0600	[thread overview]
Message-ID: <87cz9dq1dx.fsf@gmail.com> (raw)
In-Reply-To: <xmqq1qpwwwxg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:
>> Provides the same benefits to scripts as exposing GIT_EDITOR, but
>> allows distinguishing the 'sequence' editor from the 'core' editor.
>>
>> See also 44fcb4977cbae67f4698306ccfe982420ceebcbf.
>
> Why should we ;-)?

I must admit I struggled quite a bit with a useful commit message, so I
greatly appreciate the suggestion :-) I've incorporated your suggestion
for a future v2.

>> +GIT_SEQUENCE_EDITOR::
>> +    Text editor for use by Git sequencer commands. Like `GIT_EDITOR`,
>
> Do our readers know what "Git sequencer commands" are?  "rebase -i"
> of course is the primary one, but "cherry-pick" and "revert" that
> deals with multiple commits are technically "sequencer commands", as
> they also use the sequencer machinery.  But for them, the users do
> not get a chance to edit the "todo" list with their sequence editor,
> unlike "rebase -i".

That's a good point; I hadn't considered that as a potential source of
confusion -- prefering instead to future-proof the docs at the cost of
understandability :-)

> I am wondering if it is easier to understand, without losing
> technical correctness, to exactly name the command, without
> pretending as if the sequence editor is used in situations wider
> than where "rebase -i" is used, e.g.
>
> 	The text editor program used to edit the 'todo' file while
> 	running "git rebase -i".

I've incorporated your suggestion. It's possibly worth noting that I had
wanted to prefer prose ('interactive rebase') over a specific invocation
('git rebase -i'), but I see existing precedent for referring to it as
the latter in documentation (and release notes especially). I suppose
this practice is intended (either consciously or otherwise) to make it
more straightforward to cross-reference different pieces of the
documentation?

>> diff --git a/builtin/var.c b/builtin/var.c
>> index 491db274292..9a2d31dc4aa 100644
>> --- a/builtin/var.c
>> +++ b/builtin/var.c
>> @@ -19,6 +19,16 @@ static const char *editor(int flag)
>>  	return pgm;
>>  }
>>
>> +static const char *sequence_editor(int flag)
>> +{
>> +	const char *pgm = git_sequence_editor();
>> +
>> +	if (!pgm && flag & IDENT_STRICT)
>> +		die("Terminal is dumb, but EDITOR unset");
>
> I know this was copied from editor(), but the message does not make
> much sense.  It's not like the caller of read_var() is not prepared
> to see a NULL returned, so letting it return NULL would make more
> sense.  Since the ancient past back when editor() function was
> written, launch_editor() and the logic to die with "on dumb terminal
> you must specify an EDITOR" have migrated to editor.c and there is
> no strong reason to keep the corresponding die() even in editor()
> function (I do not recommend removing it as part of this topic,
> though), and adding a new one makes even less sense.

I'm glad you brought this up. To be perfectly honest, I'm not confident
I know what IDENT_STRICT is even supposed to mean -- it looks to be
undocumented in cache.h. Here's what I *think* I've been able to piece
together based on what you've said and some commit history:

    f9bc573fdaeaf8621008f3f49aaaa64869791691 suggests that setting
    IDENT_STRICT is intended to be 'more upset' about 'things' (the
    commit I mention is specifically talking about identities -- which
    explains the IDENT_* prefix) that aren't well-defined. In porcelain
    code, we want to quit immediately if there's nothing available since
    you can't really open up COMMIT_MSG, e.g., without a well-defined
    editor. Better to die early with a semi-useful message than to let
    the issue propagate downstream.

    This does not apply to git-var since the purpose of this command is
    not to invoke an editor, but to inspect configuration state via
    well-defined API. In this context, it's not necessary/appropriate to
    die early since, for the purposes of git-var, 'no configuration' is
    a perfectly valid (albeit confusing) state to be in.

I'd like to confirm this / understand more about what's going on here
before making the code change on this one. If I can understand what's
going on here well enough to write an informed commit message, I can
remove this vestigial code from editor() in a separate patch.

--
Sean Allred