diff mbox series

[RFC,1/2] commit: don't add scissors line if one exists

Message ID 1c16b9497bd630f0636aa7729082da7a90ba42d9.1542172724.git.liu.denton@gmail.com
State New, archived
Headers show
Series Fix scissors bug during merge conflict | expand

Commit Message

Denton Liu Nov. 14, 2018, 5:24 a.m. UTC
If commit.cleanup = scissors is specified, don't produce a scissors line
if one already exists in the commit message.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/commit.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Nov. 14, 2018, 8:06 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> If commit.cleanup = scissors is specified, don't produce a scissors line
> if one already exists in the commit message.

It is good that you won't have two such lines in the end result, but
is this (1) hiding real problem under the rug? (2) losing information?

If the current invocation of "git commit" added a scissors line in
the buffer to be edited already, and we are adding another one in
this function, is it possible that the real problem that somebody
else has called wt_status_add_cut_line() before this function is
called, in which case that other caller is what we need to fix,
instead of this one?

If the existing line in the buffer came from the end user (perhaps
it was given from "-F <file>", etc., with "-e" option) or --amend,
how can we be sure if it is OK to lose everything after that
scissors looking line?  In other words, the scissors looking line
may just be part of the log message, in which case we may want to
quote/escape it, so that the true scissors we append at a later
place in the buffer would be noticed without losing the text before
and after that scissors looking line we already had when this
function was called?
Denton Liu Nov. 14, 2018, 6:06 p.m. UTC | #2
On Wed, Nov 14, 2018 at 05:06:32PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > If commit.cleanup = scissors is specified, don't produce a scissors line
> > if one already exists in the commit message.
> 
> It is good that you won't have two such lines in the end result, but
> is this (1) hiding real problem under the rug? (2) losing information?
> 
> If the current invocation of "git commit" added a scissors line in
> the buffer to be edited already, and we are adding another one in
> this function, is it possible that the real problem that somebody
> else has called wt_status_add_cut_line() before this function is
> called, in which case that other caller is what we need to fix,
> instead of this one?
> 

In patch 2/2, I intentionally inserted a scissors line into MERGE_MSG so
this patch ensures that we don't get duplicate scissors.

> If the existing line in the buffer came from the end user (perhaps
> it was given from "-F <file>", etc., with "-e" option) or --amend,
> how can we be sure if it is OK to lose everything after that
> scissors looking line?  In other words, the scissors looking line
> may just be part of the log message, in which case we may want to
> quote/escape it, so that the true scissors we append at a later
> place in the buffer would be noticed without losing the text before
> and after that scissors looking line we already had when this
> function was called?
> 

With the existing behaviour, any messages that contain a scissors
looking line will get cut at the earliest scissors anyway, so I believe
that this patch would not change the behaviour. If the users were
dealing with commit messages with a scissors looking line, the current
behaviour already requires users to be extra careful to ensure that the
scissors don't get accidentally removed so in the interest of preserving
the existing behaviour, I don't think that any extra information would
be lost from this patch.
Junio C Hamano Nov. 16, 2018, 3:32 a.m. UTC | #3
Denton Liu <liu.denton@gmail.com> writes:

>> If the current invocation of "git commit" added a scissors line in
>> the buffer to be edited already, and we are adding another one in
>> this function, is it possible that the real problem that somebody
>> else has called wt_status_add_cut_line() before this function is
>> called, in which case that other caller is what we need to fix,
>> instead of this one?
>> 
>
> In patch 2/2, I intentionally inserted a scissors line into MERGE_MSG so
> this patch ensures that we don't get duplicate scissors.

That is exactly what the paragraph you are responding to questions.
Is the code that adds a scissors line before this function is called
done the right way?  Shouldn't it be doing something differnetly?
Looking for an existing scissors looking line in this function does
not let this function differenciate two cases, i.e. we deliberately
added one already before calling this function (in which case this
function should not add another one), or we didn't add anything on
our own, but the material supplied by the end user had one (in which
case, not adding ours is losing information---imagine that the user
notices a scissors-looking line that came from the original maerial
and want to munge it, as it is part of proper message, so that it
would remain in the committed result, but because [PATCH 1/2]
stopped adding a scissors line at the right location, the user would
have to guess where to add one).

There must be an explicit way (e.g. a bit in a flag word parameter
given to this function) for the caller who knows when the new code
in [PATCH 2/2] triggers, to tell this function not to add another
one, instead of a sloppy (and less efficient) "lets's scan to see if
there already is a scissors looking line".

> With the existing behaviour, any messages that contain a scissors
> looking line will get cut at the earliest scissors anyway, so I believe
> that this patch would not change the behaviour. If the users were
> dealing with commit messages with a scissors looking line, the current
> behaviour already requires users to be extra careful to ensure that the
> scissors don't get accidentally removed so in the interest of preserving
> the existing behaviour, I don't think that any extra information would
> be lost from this patch.

Doing the "is there already a scissors looing line" approach will
*make* it harder to fix that issue, so the patch is making things
worse.
diff mbox series

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29e..e486246329 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -659,6 +659,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg2 = NULL;
 	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
 	int old_display_comment_prefix;
+	int contains_scissors = 0;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
@@ -742,6 +743,10 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		clean_message_contents = 0;
 	}
 
+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+		wt_status_locate_end(sb.buf, sb.len) != sb.len)
+	    contains_scissors = 1;
+
 	/*
 	 * The remaining cases don't modify the template message, but
 	 * just set the argument(s) to the prepare-commit-msg hook.
@@ -798,7 +803,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct ident_split ci, ai;
 
 		if (whence != FROM_COMMIT) {
-			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+				!contains_scissors)
 				wt_status_add_cut_line(s->fp);
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 			    whence == FROM_MERGE
@@ -824,7 +830,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 				  " Lines starting\nwith '%c' will be ignored, and an empty"
 				  " message aborts the commit.\n"), comment_line_char);
 		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
-			 whence == FROM_COMMIT)
+			 whence == FROM_COMMIT &&
+			 !contains_scissors)
 			wt_status_add_cut_line(s->fp);
 		else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
 			status_printf(s, GIT_COLOR_NORMAL,