| Message ID | 4f97933f173220544a5be2bf05c2bee2b044d2b1.1709024540.git.josh@joshtriplett.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v | expand |
Josh Triplett <josh@joshtriplett.org> writes: > diff --git a/wt-status.c b/wt-status.c > index ea13f5d8db..2d576f7a44 100644 I do not seem to have the preimage ea13f5d8db; as a bugfix patch, it would be preferrable to make the patches not to depend on anything in flight. If feasible, it may even be nicer to base them on one of the maintenance tracks. I managed to wiggle the patch in (somehow a context line was misindented), so there hopefully is no need to resend. Thanks.
On Tue, Feb 27, 2024 at 09:43:21AM -0800, Junio C Hamano wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > > diff --git a/wt-status.c b/wt-status.c > > index ea13f5d8db..2d576f7a44 100644 > > I do not seem to have the preimage ea13f5d8db; as a bugfix patch, it > would be preferrable to make the patches not to depend on anything > in flight. If feasible, it may even be nicer to base them on one of > the maintenance tracks. > > I managed to wiggle the patch in (somehow a context line was > misindented), so there hopefully is no need to resend. Sorry about that. I had these two patches in the same branch as my other recent patch `advice: Add advice.scissors to suppress "do not modify or remove this line"` but the two ended up independent so I didn't send them as a series. That patch and this two-patch series should be applicable independently. If you do end up needing a resend of any of them, I'm happy to do so.
Josh Triplett <josh@joshtriplett.org> writes:
> If you do end up needing a resend of any of them, I'm happy to do so.
I do not think there is need for resending, but I think you promised
to add some tests earlier, so an updated patch may be in order ;-)
Thanks.
On Wed, Feb 28, 2024 at 09:41:22PM -0800, Junio C Hamano wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > If you do end up needing a resend of any of them, I'm happy to do so. > > I do not think there is need for resending, but I think you promised > to add some tests earlier, so an updated patch may be in order ;-) I did add a test; v2 that you replied to has this: > Add a test for this. [...] > t/t7502-commit-porcelain.sh | 5 +++++ [...] > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh > index a87c211d0b..b37e2018a7 100755 > --- a/t/t7502-commit-porcelain.sh > +++ b/t/t7502-commit-porcelain.sh > @@ -736,6 +736,11 @@ test_expect_success 'message shows date when it is explicitly set' ' > .git/COMMIT_EDITMSG > ' > > +test_expect_success 'message does not have multiple scissors lines' ' > + git commit --cleanup=scissors -v --allow-empty -e -m foo && > + test $(grep -c -e "--- >8 ---" .git/COMMIT_EDITMSG) -eq 1 > +' > + > test_expect_success AUTOIDENT 'message shows committer when it is automatic' ' > > echo >>negative && https://lore.kernel.org/git/xmqqedcxvnn8.fsf@gitster.g/T/#Z2e.:..:4f97933f173220544a5be2bf05c2bee2b044d2b1.1709024540.git.josh::40joshtriplett.org:1t:t7502-commit-porcelain.sh
Josh Triplett <josh@joshtriplett.org> writes: > I did add a test; v2 that you replied to has this: >> Add a test for this. Thanks. Let's mark it for 'next' then.
diff --git a/builtin/commit.c b/builtin/commit.c index 6d1fa71676..e0a6d43179 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -926,7 +926,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, if (whence != FROM_COMMIT) { if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS && !merge_contains_scissors) - wt_status_add_cut_line(s->fp); + wt_status_add_cut_line(s); status_printf_ln( s, GIT_COLOR_NORMAL, whence == FROM_MERGE ? @@ -947,7 +947,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char); else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { if (whence == FROM_COMMIT && !merge_contains_scissors) - wt_status_add_cut_line(s->fp); + wt_status_add_cut_line(s); } else /* COMMIT_MSG_CLEANUP_SPACE, that is. */ status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char); diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh index a87c211d0b..b37e2018a7 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -736,6 +736,11 @@ test_expect_success 'message shows date when it is explicitly set' ' .git/COMMIT_EDITMSG ' +test_expect_success 'message does not have multiple scissors lines' ' + git commit --cleanup=scissors -v --allow-empty -e -m foo && + test $(grep -c -e "--- >8 ---" .git/COMMIT_EDITMSG) -eq 1 +' + test_expect_success AUTOIDENT 'message shows committer when it is automatic' ' echo >>negative && diff --git a/wt-status.c b/wt-status.c index ea13f5d8db..2d576f7a44 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1108,12 +1108,15 @@ void wt_status_append_cut_line(struct strbuf *buf) strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char); } -void wt_status_add_cut_line(FILE *fp) +void wt_status_add_cut_line(struct wt_status *s) { struct strbuf buf = STRBUF_INIT; + if (s->added_cut_line) + return; + s->added_cut_line = 1; wt_status_append_cut_line(&buf); - fputs(buf.buf, fp); + fputs(buf.buf, s->fp); strbuf_release(&buf); } @@ -1144,11 +1147,12 @@ 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); + wt_status_add_cut_line(s); } 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..5e99ba4707 100644 --- a/wt-status.h +++ b/wt-status.h @@ -130,6 +130,7 @@ struct wt_status { int rename_score; int rename_limit; enum wt_status_format status_format; + unsigned char added_cut_line; /* boolean */ struct wt_status_state state; struct object_id oid_commit; /* when not Initial */ @@ -147,7 +148,7 @@ struct wt_status { size_t wt_status_locate_end(const char *s, size_t len); void wt_status_append_cut_line(struct strbuf *buf); -void wt_status_add_cut_line(FILE *fp); +void wt_status_add_cut_line(struct wt_status *s); void wt_status_prepare(struct repository *r, struct wt_status *s); void wt_status_print(struct wt_status *s); void wt_status_collect(struct wt_status *s);
`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. Make wt_status_add_cut_line remember if it has added a cut line before, and avoid adding a redundant one. Add a test for this. Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- v2: Make wt_status_add_cut_line remember if it has added a cut line, rather than making later callers try to figure out if it has been called before. Add a test. Note that other parts of the code do already try to figure out if the *merge* logic has added scissors already, which is where merge_contains_scissors comes from. Patch 2/2 unifies that machinery. builtin/commit.c | 4 ++-- t/t7502-commit-porcelain.sh | 5 +++++ wt-status.c | 12 ++++++++---- wt-status.h | 3 ++- 4 files changed, 17 insertions(+), 7 deletions(-)