| Message ID | 9c09cea2679e14258720ee63e932e3b9459dbd8c.1708921369.git.josh@joshtriplett.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit: Avoid redundant scissor line with --cleanup=scissors -v | expand |
Josh Triplett <josh@joshtriplett.org> writes: > `git commit --cleanup=scissors -v` currently prints two scissors lines: > one at the start of the comment lines, and the other right before the > diff. This is redundant, and pushes the diff further down in the user's > editor than it needs to be. Interesting discovery. > diff --git a/wt-status.c b/wt-status.c > index b5a29083df..459d399baa 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s) > * file (and even the "auto" setting won't work, since it > * will have checked isatty on stdout). But we then do want > * to insert the scissor line here to reliably remove the > - * diff before committing. > + * diff before committing, if we didn't already include one > + * before. > */ > if (s->fp != stdout) { > rev.diffopt.use_color = 0; > - wt_status_add_cut_line(s->fp); > + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS) > + wt_status_add_cut_line(s->fp); > } The machinery to populate the log message buffer should ideally be taught to remember if it already has added a scissors-line and to refrain from adding redundant ones. That way, we do not have to rely on the order of places that make wt_status_add_cut_line() calls or what condition they use to decide to make these calls. This hunk for example knows not just this one produces cut-line after the other one potentially added one, but also the logic used by the other one to decide to add one, which is even worse. I find the solution presented here a bit unsatisfactory, for this reason, but for now it may be OK, as we probably are not adding any more places and conditions to emit a scissors line. > builtin/commit.c | 2 ++ > sequencer.h | 7 ------- > wt-status.c | 6 ++++-- > wt-status.h | 8 ++++++++ > 4 files changed, 14 insertions(+), 9 deletions(-) If this change did not break any existing tests that checked the combination of options and output when they are used together, it means we have a gap in the test coverage. We needs a test or two to protect this fix from future breakages. Thanks.
On Mon, Feb 26, 2024 at 10:03:22AM -0800, Junio C Hamano wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > > `git commit --cleanup=scissors -v` currently prints two scissors lines: > > one at the start of the comment lines, and the other right before the > > diff. This is redundant, and pushes the diff further down in the user's > > editor than it needs to be. > > Interesting discovery. > > > diff --git a/wt-status.c b/wt-status.c > > index b5a29083df..459d399baa 100644 > > --- a/wt-status.c > > +++ b/wt-status.c > > @@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s) > > * file (and even the "auto" setting won't work, since it > > * will have checked isatty on stdout). But we then do want > > * to insert the scissor line here to reliably remove the > > - * diff before committing. > > + * diff before committing, if we didn't already include one > > + * before. > > */ > > if (s->fp != stdout) { > > rev.diffopt.use_color = 0; > > - wt_status_add_cut_line(s->fp); > > + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS) > > + wt_status_add_cut_line(s->fp); > > } > > The machinery to populate the log message buffer should ideally be > taught to remember if it already has added a scissors-line and to > refrain from adding redundant ones. That way, we do not have to > rely on the order of places that make wt_status_add_cut_line() calls > or what condition they use to decide to make these calls. > > This hunk for example knows not just this one produces cut-line > after the other one potentially added one, but also the logic used > by the other one to decide to add one, which is even worse. I find > the solution presented here a bit unsatisfactory, for this reason, > but for now it may be OK, as we probably are not adding any more > places and conditions to emit a scissors line. I could add statefulness to wt_status_add_cut_line instead, on the assumption that it's the only thing that should be adding a cut line, and having it not add the line if previously added. For instance, it could accept a pointer to the full wt_status rather than just the fp, and keep a boolean state there. > > builtin/commit.c | 2 ++ > > sequencer.h | 7 ------- > > wt-status.c | 6 ++++-- > > wt-status.h | 8 ++++++++ > > 4 files changed, 14 insertions(+), 9 deletions(-) > > If this change did not break any existing tests that checked the > combination of options and output when they are used together, it > means we have a gap in the test coverage. We needs a test or two > to protect this fix from future breakages. I did run the testsuite, and it passed. I can add a simple test easily enough.
Josh Triplett <josh@joshtriplett.org> writes: > I could add statefulness to wt_status_add_cut_line instead, on the > assumption that it's the only thing that should be adding a cut line, > and having it not add the line if previously added. For instance, it > could accept a pointer to the full wt_status rather than just the fp, > and keep a boolean state there. Yeah, that approach also has to assume that wt_status structure is used only once to create a single message buffer without being reused, but I think that is a safe assumption, too. The function being the only thing that adds the scissors line should also be a safe assumption in code hygiene standpoint---if somebody else tries to manually write such a line, we'll shoot such a patch down and tell them to call this function anyway ;-). > I did run the testsuite, and it passed. I can add a simple test easily > enough. It would be prudent to do so. Thanks.
diff --git a/builtin/commit.c b/builtin/commit.c index 6d1fa71676..6b2b412932 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -888,6 +888,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, */ s->hints = 0; + s->cleanup_mode = cleanup_mode; + if (clean_message_contents) strbuf_stripspace(&sb, '\0'); diff --git a/sequencer.h b/sequencer.h index dcef7bb99c..9f818e96f0 100644 --- a/sequencer.h +++ b/sequencer.h @@ -22,13 +22,6 @@ enum replay_action { REPLAY_INTERACTIVE_REBASE }; -enum commit_msg_cleanup_mode { - COMMIT_MSG_CLEANUP_SPACE, - COMMIT_MSG_CLEANUP_NONE, - COMMIT_MSG_CLEANUP_SCISSORS, - COMMIT_MSG_CLEANUP_ALL -}; - struct replay_opts { enum replay_action action; diff --git a/wt-status.c b/wt-status.c index b5a29083df..459d399baa 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s) * file (and even the "auto" setting won't work, since it * will have checked isatty on stdout). But we then do want * to insert the scissor line here to reliably remove the - * diff before committing. + * diff before committing, if we didn't already include one + * before. */ if (s->fp != stdout) { rev.diffopt.use_color = 0; - wt_status_add_cut_line(s->fp); + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS) + wt_status_add_cut_line(s->fp); } if (s->verbose > 1 && s->committable) { /* print_updated() printed a header, so do we */ diff --git a/wt-status.h b/wt-status.h index 819dcad723..5ede705e93 100644 --- a/wt-status.h +++ b/wt-status.h @@ -22,6 +22,13 @@ enum color_wt_status { WT_STATUS_MAXSLOT }; +enum commit_msg_cleanup_mode { + COMMIT_MSG_CLEANUP_SPACE, + COMMIT_MSG_CLEANUP_NONE, + COMMIT_MSG_CLEANUP_SCISSORS, + COMMIT_MSG_CLEANUP_ALL +}; + enum untracked_status_type { SHOW_NO_UNTRACKED_FILES, SHOW_NORMAL_UNTRACKED_FILES, @@ -130,6 +137,7 @@ struct wt_status { int rename_score; int rename_limit; enum wt_status_format status_format; + enum commit_msg_cleanup_mode cleanup_mode; struct wt_status_state state; struct object_id oid_commit; /* when not Initial */
`git commit --cleanup=scissors -v` currently prints two scissors lines: one at the start of the comment lines, and the other right before the diff. This is redundant, and pushes the diff further down in the user's editor than it needs to be. Pass the cleanup mode into wt_status, so that wt_status_print can avoid printing the extra scissors if already printed. This moves the enum commit_msg_cleanup_mode from sequencer.h to wt-status.h to allow wt_status to use the type. sequencer.h already includes wt-status.h, so this doesn't affect anything else. Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- builtin/commit.c | 2 ++ sequencer.h | 7 ------- wt-status.c | 6 ++++-- wt-status.h | 8 ++++++++ 4 files changed, 14 insertions(+), 9 deletions(-)