| Message ID | 0a7b9172add0a0107e0765a59a798b92161788dd.1708921148.git.josh@joshtriplett.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | advice: Add advice.scissors to suppress "do not modify or remove this line" | expand |
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.
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.
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.
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
<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.
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.
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.
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)
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(-)