git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] var: add GIT_SEQUENCE_EDITOR variable
@ 2022-11-20 19:20 Sean Allred via GitGitGadget
  2022-11-21  8:09 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Allred via GitGitGadget @ 2022-11-20 19:20 UTC (permalink / raw)
  To: git; +Cc: Sean Allred, Sean Allred

From: Sean Allred <allred.sean@gmail.com>

Provides the same benefits to scripts as exposing GIT_EDITOR, but
allows distinguishing the 'sequence' editor from the 'core' editor.

See also 44fcb4977cbae67f4698306ccfe982420ceebcbf.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
    var: add GIT_SEQUENCE_EDITOR variable
    
    In my case, I'm overriding the sequence editor in git rebase -i to do
    some pre-processing on the todo file, but I'd still like to open up the
    editor for further manipulation/verification of the todo steps. Just
    using GIT_EDITOR here will clobber folks' expectations that their
    custom-built sequence editor pops up (e.g. VSCode or Git Cola).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1424%2Fvermiculus%2Fsa%2Fgit-var-sequence-editor-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1424/vermiculus/sa/git-var-sequence-editor-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1424

 Documentation/git-var.txt |  7 +++++++
 builtin/var.c             | 11 +++++++++++
 t/t0007-git-var.sh        | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 6aa521fab23..764a94b2a1f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -49,6 +49,13 @@ ifdef::git-default-editor[]
     The build you are using chose '{git-default-editor}' as the default.
 endif::git-default-editor[]
 
+GIT_SEQUENCE_EDITOR::
+    Text editor for use by Git sequencer commands. Like `GIT_EDITOR`,
+    the value is meant to be interpreted by the shell when it is used.
+    The order of preference is the `$GIT_SEQUENCE_EDITOR` environment
+    variable, then `sequence.editor` configuration, and then the value
+    of `git var GIT_EDITOR`.
+
 GIT_PAGER::
     Text viewer for use by Git commands (e.g., 'less').  The value
     is meant to be interpreted by the shell.  The order of preference
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");
+
+	return pgm;
+}
+
 static const char *pager(int flag)
 {
 	const char *pgm = git_pager(1);
@@ -41,6 +51,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
+	{ "GIT_SEQUENCE_EDITOR", sequence_editor },
 	{ "GIT_PAGER", pager },
 	{ "GIT_DEFAULT_BRANCH", default_branch },
 	{ "", NULL },
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e56f4b9ac59..3199285fa7b 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -47,6 +47,44 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
 	)
 '
 
+test_expect_success 'get GIT_SEQUENCE_EDITOR without configuration' '
+	(
+		sane_unset GIT_SEQUENCE_EDITOR &&
+		git var GIT_EDITOR >expect &&
+		git var GIT_SEQUENCE_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration' '
+	test_config sequence.editor foo &&
+	(
+		sane_unset GIT_SEQUENCE_EDITOR &&
+		echo foo >expect &&
+		git var GIT_SEQUENCE_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_SEQUENCE_EDITOR with environment variable' '
+	(
+		sane_unset GIT_SEQUENCE_EDITOR &&
+		echo bar >expect &&
+		GIT_SEQUENCE_EDITOR=bar git var GIT_SEQUENCE_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment variable' '
+	test_config sequence.editor foo &&
+	(
+		sane_unset GIT_SEQUENCE_EDITOR &&
+		echo bar >expect &&
+		GIT_SEQUENCE_EDITOR=bar git var GIT_SEQUENCE_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

base-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] var: add GIT_SEQUENCE_EDITOR variable
  2022-11-20 19:20 [PATCH] var: add GIT_SEQUENCE_EDITOR variable Sean Allred via GitGitGadget
@ 2022-11-21  8:09 ` Junio C Hamano
  2022-11-23 12:21   ` Sean Allred
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2022-11-21  8:09 UTC (permalink / raw)
  To: Sean Allred via GitGitGadget; +Cc: git, Sean Allred, Sean Allred

"Sean Allred via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Allred <allred.sean@gmail.com>
>
> 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 ;-)?

If you explain why "git var GIT_SEQUENCE_EDITOR" is useful in a more
direct way, you do not even have to refer to a long hexadecimal string
which by itself does not mean anything to sane human beings.

    The editor program used by Git when editing the sequencer "todo"
    file is determined by examining a few environment variables and
    also affected by configuration variables.  Introduce "git var
    GIT_SEQUENCE_EDITOR" that gives users an access to the final
    result of the logic without having to know the exact detail.

    This is very similar in spirit to 44fcb497 (Teach git var about
    GIT_EDITOR, 2009-11-11) that introduced "git var GIT_EDITOR".

or something like that, perhaps?

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

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

or something.

> +    the value is meant to be interpreted by the shell when it is used.
> +    The order of preference is the `$GIT_SEQUENCE_EDITOR` environment
> +    variable, then `sequence.editor` configuration, and then the value
> +    of `git var GIT_EDITOR`.

OK.

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] var: add GIT_SEQUENCE_EDITOR variable
  2022-11-21  8:09 ` Junio C Hamano
@ 2022-11-23 12:21   ` Sean Allred
  0 siblings, 0 replies; 3+ messages in thread
From: Sean Allred @ 2022-11-23 12:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sean Allred via GitGitGadget, git, Sean Allred


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-11-23 13:10 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-20 19:20 [PATCH] var: add GIT_SEQUENCE_EDITOR variable Sean Allred via GitGitGadget
2022-11-21  8:09 ` Junio C Hamano
2022-11-23 12:21   ` Sean Allred

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).