Re: [PATCH] var: add GIT_SEQUENCE_EDITOR variable

From: Sean Allred <allred.sean@gmail.com>
Date: Nov 23, 2022, 12:21 UTC
Message-ID: <87cz9dq1dx.fsf@gmail.com>
In-Reply-To: <xmqq1qpwwwxg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 6 quoted lines

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

Show 9 quoted lines

>> +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 :-)

Show 7 quoted lines

> 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?

Show 24 quoted lines

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