[PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Josh Triplett]

The scissors line before the diff in a verbose commit, or above all the
comments when using --cleanup=scissors, has the following two lines of
explanation after it:

Do not modify or remove the line above.
Everything below it will be ignored.

This is useful advice for new users, but potentially redundant for
experienced users, who might instead appreciate seeing two more lines of
information in their editor.

Add advice.scissors to suppress that explanation.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 Documentation/config/advice.txt | 5 +++++
 advice.c                        | 1 +
 advice.h                        | 1 +
 wt-status.c                     | 3 ++-
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2..33ab688b6c 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -104,6 +104,11 @@ advice.*::
 	rmHints::
 		In case of failure in the output of linkgit:git-rm[1],
 		show directions on how to proceed from the current state.
+	scissors::
+		Advice shown by linkgit:git-commit[1] in the commit message
+		opened in an editor, after a scissors line (containing >8),
+		saying not to remove the line and that everything after the line
+		will be ignored.
 	sequencerInUse::
 		Advice shown when a sequencer command is already in progress.
 	skippedCherryPicks::
diff --git a/advice.c b/advice.c
index 6e9098ff08..0588012562 100644
--- a/advice.c
+++ b/advice.c
@@ -71,6 +71,7 @@ static struct {
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
 	[ADVICE_RM_HINTS]				= { "rmHints" },
+	[ADVICE_SCISSORS]				= { "scissors" },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38..9725aa4199 100644
--- a/advice.h
+++ b/advice.h
@@ -39,6 +39,7 @@ enum advice_type {
 	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
+	ADVICE_SCISSORS,
 	ADVICE_SEQUENCER_IN_USE,
 	ADVICE_SET_UPSTREAM_FAILURE,
 	ADVICE_SKIPPED_CHERRY_PICKS,
diff --git a/wt-status.c b/wt-status.c
index 459d399baa..19d4986351 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1104,7 +1104,8 @@ void wt_status_append_cut_line(struct strbuf *buf)
 	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
 
 	strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
-	strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+	if (advice_enabled(ADVICE_SCISSORS))
+		strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
 }
 
 void wt_status_add_cut_line(FILE *fp)
-- 
2.43.0

Re: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Josh Triplett]

On Sun, Feb 25, 2024 at 08:21:54PM -0800, Josh Triplett wrote:
> The scissors line before the diff in a verbose commit, or above all the
> comments when using --cleanup=scissors, has the following two lines of
> explanation after it:
> 
> Do not modify or remove the line above.
> Everything below it will be ignored.
> 
> This is useful advice for new users, but potentially redundant for
> experienced users, who might instead appreciate seeing two more lines of
> information in their editor.
> 
> Add advice.scissors to suppress that explanation.

Following up on this patch. Happy to rework if needed.

Re: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> On Sun, Feb 25, 2024 at 08:21:54PM -0800, Josh Triplett wrote:
>> The scissors line before the diff in a verbose commit, or above all the
>> comments when using --cleanup=scissors, has the following two lines of
>> explanation after it:
>> 
>> Do not modify or remove the line above.
>> Everything below it will be ignored.
>> 
>> This is useful advice for new users, but potentially redundant for
>> experienced users, who might instead appreciate seeing two more lines of
>> information in their editor.
>> 
>> Add advice.scissors to suppress that explanation.
>
> Following up on this patch. Happy to rework if needed.

I am not personally interested in the feature myself, and I doubt it
would help the end-user experience very much.  You'd need to find
somebody else to cheer for the topic ;-)

Thanks.

RE: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[rsbecker]

On Tuesday, April 16, 2024 4:36 PM, Junio C Hamano wrote:
>Josh Triplett <josh@joshtriplett.org> writes:
>
>> On Sun, Feb 25, 2024 at 08:21:54PM -0800, Josh Triplett wrote:
>>> The scissors line before the diff in a verbose commit, or above all
>>> the comments when using --cleanup=scissors, has the following two
>>> lines of explanation after it:
>>>
>>> Do not modify or remove the line above.
>>> Everything below it will be ignored.
>>>
>>> This is useful advice for new users, but potentially redundant for
>>> experienced users, who might instead appreciate seeing two more lines
>>> of information in their editor.
>>>
>>> Add advice.scissors to suppress that explanation.
>>
>> Following up on this patch. Happy to rework if needed.
>
>I am not personally interested in the feature myself, and I doubt it would
help the
>end-user experience very much.  You'd need to find somebody else to cheer
for the
>topic ;-)

I am having a bit of trouble understanding the use-case for this. Is it
limited to linkgit? Under what circumstances would I need to use such
capabilities?
Thanks,
Randall

Re: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

>>>> This is useful advice for new users, but potentially redundant for
>>>> experienced users, who might instead appreciate seeing two more lines
>>>> of information in their editor.
>>>>
>>>> Add advice.scissors to suppress that explanation.
>>>
>>> Following up on this patch. Happy to rework if needed.
>>
>>I am not personally interested in the feature myself, and I doubt it would
> help the
>>end-user experience very much.  You'd need to find somebody else to cheer
> for the
>>topic ;-)
>
> I am having a bit of trouble understanding the use-case for
> this. Is it limited to linkgit? Under what circumstances would I
> need to use such capabilities?

When you run "git commit" from the command line without specifying
any message, you'd get an editor spawned for you with something like
this in the file ("--- >8 ---" and "--- 8< ---" are for illustration
purposes in this message).

    --- >8 ---

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    #
    # On branch next
    # Untracked files:
    #	+runme.sh
    #	P
    #
    --- 8< ---

But when you run "git commit --cleanup=scissors" (or use the
equivalent configuration variables), you'd get this instead.

    --- >8 ---

    # ------------------------ >8 ------------------------
    # Do not modify or remove the line above.
    # Everything below it will be ignored.
    #
    # On branch next
    # Untracked files:
    #	+runme.sh
    #	P
    #
    --- 8< ---

The new advice configuration is to suppress the two lines from that
message template.

I agree that a bit more background information should be given in
the proposed log message to help readers.  It should mention when
this new setting is relevant (e.g., when the "--cleanup=scissors"
option is in effect), at least.

I suspect that the same configuration variable wants to also control
the "helpful" comment in the "normal" case, in which case the name
of the configuration variable would need to be rethought.

RE: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[rsbecker]

On Tuesday, April 16, 2024 6:31 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>>>>> This is useful advice for new users, but potentially redundant for
>>>>> experienced users, who might instead appreciate seeing two more
>>>>> lines of information in their editor.
>>>>>
>>>>> Add advice.scissors to suppress that explanation.
>>>>
>>>> Following up on this patch. Happy to rework if needed.
>>>
>>>I am not personally interested in the feature myself, and I doubt it
>>>would
>> help the
>>>end-user experience very much.  You'd need to find somebody else to
>>>cheer
>> for the
>>>topic ;-)
>>
>> I am having a bit of trouble understanding the use-case for this. Is
>> it limited to linkgit? Under what circumstances would I need to use
>> such capabilities?
>
>When you run "git commit" from the command line without specifying any
>message, you'd get an editor spawned for you with something like this in
the file ("--
>- >8 ---" and "--- 8< ---" are for illustration purposes in this message).
>
>    --- >8 ---
>
>    # Please enter the commit message for your changes. Lines starting
>    # with '#' will be ignored, and an empty message aborts the commit.
>    #
>    # On branch next
>    # Untracked files:
>    #	+runme.sh
>    #	P
>    #
>    --- 8< ---
>
>But when you run "git commit --cleanup=scissors" (or use the equivalent
>configuration variables), you'd get this instead.
>
>    --- >8 ---
>
>    # ------------------------ >8 ------------------------
>    # Do not modify or remove the line above.
>    # Everything below it will be ignored.
>    #
>    # On branch next
>    # Untracked files:
>    #	+runme.sh
>    #	P
>    #
>    --- 8< ---
>
>The new advice configuration is to suppress the two lines from that message
>template.
>
>I agree that a bit more background information should be given in the
proposed log
>message to help readers.  It should mention when this new setting is
relevant (e.g.,
>when the "--cleanup=scissors"
>option is in effect), at least.
>
>I suspect that the same configuration variable wants to also control the
"helpful"
>comment in the "normal" case, in which case the name of the configuration
variable
>would need to be rethought.

Thanks. I hope that the default "helpful" comment in the "normal" case would
not change anything. I personally have course material that references
examples with the current commit message structure that would be impacted -
No problem changing it, but I am not sure what the new result would be.

As a comment on this, the scissors option might need some NLS consideration
in organizations where there are more than one languages/encodings in use. I
could see some global settings conflicting with local, but that would need
documentation.

Re: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Dragan Simic]

Hello all,

Please see my comments below.

On 2024-04-17 00:31, Junio C Hamano wrote:
> <rsbecker@nexbridge.com> writes:
>> I am having a bit of trouble understanding the use-case for
>> this. Is it limited to linkgit? Under what circumstances would I
>> need to use such capabilities?
> 
> When you run "git commit" from the command line without specifying
> any message, you'd get an editor spawned for you with something like
> this in the file ("--- >8 ---" and "--- 8< ---" are for illustration
> purposes in this message).
> 
>     --- >8 ---
> 
>     # Please enter the commit message for your changes. Lines starting
>     # with '#' will be ignored, and an empty message aborts the commit.
>     #
>     # On branch next
>     # Untracked files:
>     #	+runme.sh
>     #	P
>     #
>     --- 8< ---

Frankly, I'd much rather see a new configuration option that would
suppress the two leading lines in the example above.  Of course, they'd
still be displayed by default.

> But when you run "git commit --cleanup=scissors" (or use the
> equivalent configuration variables), you'd get this instead.
> 
>     --- >8 ---
> 
>     # ------------------------ >8 ------------------------
>     # Do not modify or remove the line above.
>     # Everything below it will be ignored.
>     #
>     # On branch next
>     # Untracked files:
>     #	+runme.sh
>     #	P
>     #
>     --- 8< ---
> 
> The new advice configuration is to suppress the two lines from that
> message template.

I think this new configuration option might be useful to some users,
but I'd suggest that it gets extended to also suppress the two opening
lines mentioned in my comment on the first example above.

> I agree that a bit more background information should be given in
> the proposed log message to help readers.  It should mention when
> this new setting is relevant (e.g., when the "--cleanup=scissors"
> option is in effect), at least.
> 
> I suspect that the same configuration variable wants to also control
> the "helpful" comment in the "normal" case, in which case the name
> of the configuration variable would need to be rethought.

Agreed, a better name for the new option would be helpful.

Re: [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"

[Rubén Justo]

On Sun, Feb 25, 2024 at 08:21:51PM -0800, Josh Triplett wrote:
> The scissors line before the diff in a verbose commit, or above all the
> comments when using --cleanup=scissors, has the following two lines of
> explanation after it:
> 
> Do not modify or remove the line above.
> Everything below it will be ignored.
> 
> This is useful advice for new users, but potentially redundant for
> experienced users, who might instead appreciate seeing two more lines of
> information in their editor.

Sounds sensible.

> 
> Add advice.scissors to suppress that explanation.

Perhaps "advice.scissorsHint" is a better name?  I'm very bad at
choosing names, but just "scissors" seems too generic to me.

> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  Documentation/config/advice.txt | 5 +++++
>  advice.c                        | 1 +
>  advice.h                        | 1 +
>  wt-status.c                     | 3 ++-
>  4 files changed, 9 insertions(+), 1 deletion(-)

Some tests would be desirable, to ensure that this keeps working in the
future.

> 
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index c7ea70f2e2..33ab688b6c 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -104,6 +104,11 @@ advice.*::
>  	rmHints::
>  		In case of failure in the output of linkgit:git-rm[1],
>  		show directions on how to proceed from the current state.
> +	scissors::

Good.  After "rmHints" and before "sequencerInUse".  Looks like the
right position for the new name.

> +		Advice shown by linkgit:git-commit[1] in the commit message
> +		opened in an editor, after a scissors line (containing >8),
> +		saying not to remove the line and that everything after the line
> +		will be ignored.
>  	sequencerInUse::
>  		Advice shown when a sequencer command is already in progress.
>  	skippedCherryPicks::
> diff --git a/advice.c b/advice.c
> index 6e9098ff08..0588012562 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -71,6 +71,7 @@ static struct {
>  	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
>  	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
>  	[ADVICE_RM_HINTS]				= { "rmHints" },
> +	[ADVICE_SCISSORS]				= { "scissors" },

Ditto.

>  	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
>  	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
>  	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
> diff --git a/advice.h b/advice.h
> index 9d4f49ae38..9725aa4199 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -39,6 +39,7 @@ enum advice_type {
>  	ADVICE_RESET_NO_REFRESH_WARNING,
>  	ADVICE_RESOLVE_CONFLICT,
>  	ADVICE_RM_HINTS,
> +	ADVICE_SCISSORS,

Ditto.

>  	ADVICE_SEQUENCER_IN_USE,
>  	ADVICE_SET_UPSTREAM_FAILURE,
>  	ADVICE_SKIPPED_CHERRY_PICKS,
> diff --git a/wt-status.c b/wt-status.c
> index 459d399baa..19d4986351 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1104,7 +1104,8 @@ void wt_status_append_cut_line(struct strbuf *buf)
>  	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
>  
>  	strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
> -	strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
> +	if (advice_enabled(ADVICE_SCISSORS))
> +		strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);

I wonder if advise_if_enabled() might have a chance here.  I'm just
thinking out loud.  "if(advice_enabled(.." is fine.

>  }
>  
>  void wt_status_add_cut_line(FILE *fp)
> -- 
> 2.43.0
> 

Thanks.