Junio C Hamano <gitster@xxxxxxxxx> 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
