[RFC,0/2] Fix scissors bug during merge conflict

Message

Denton Liu Nov. 14, 2018, 5:24 a.m. UTC

I discovered a bug in Git a while ago where if one is using
commit.cleanup = scissors, if making a commit after a merge conflict,
the scissors line will be placed after the `Conflicts:` section. As a
result, a careless Git user (e.g. me) may accidentally commit the
`Conflicts:` section.

Here is a test case to replicate the behaviour:

	mkdir test
	cd test/
	git init
	git config commit.cleanup scissors
	touch a
	git add a
	git commit -m 'test'
	echo a > a
	git commit -am 'test2'
	git checkout -b new HEAD^
	echo b > a
	git commit -am 'test3'
	git merge master
	echo c > a
	git add a
	git commit # look at the commit message here

And the resulting message that's sent to the text editor:

	Merge branch 'master' into new

	# Conflicts:
	#	a
	# ------------------------ >8 ------------------------
	# Do not modify or remove the line above.
	# Everything below it will be ignored.
	#
	# It looks like you may be committing a merge.
	# If this is not correct, please remove the file
	#	.git/MERGE_HEAD
	# and try again.


	# Please enter the commit message for your changes. Lines starting
	# with '#' will be kept; you may remove them yourself if you want to.
	# An empty message aborts the commit.
	#
	# On branch new
	# All conflicts fixed but you are still merging.
	#
	# Changes to be committed:
	#	modified:   a
	#

With this fix, the message becomes the following:

	Merge branch 'master' into new

	# ------------------------ >8 ------------------------
	# Do not modify or remove the line above.
	# Everything below it will be ignored.
	#
	# Conflicts:
	#	a
	#
	# It looks like you may be committing a merge.
	# If this is not correct, please remove the file
	#	.git/MERGE_HEAD
	# and try again.


	# Please enter the commit message for your changes. Lines starting
	# with '#' will be kept; you may remove them yourself if you want to.
	# An empty message aborts the commit.
	#
	# On branch new
	# All conflicts fixed but you are still merging.
	#
	# Changes to be committed:
	#	modified:   a
	#

Let me know what you think of the change. Of course, documentation and
testing will come after this leaves the RFC phase.

Denton Liu (2):
  commit: don't add scissors line if one exists
  merge: add scissors line on merge conflict

 builtin/commit.c | 11 +++++++++--
 builtin/merge.c  | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Nov. 14, 2018, 7:52 a.m. UTC | #1

Denton Liu <[e-mail]> writes:

> With this fix, the message becomes the following:
>
> 	Merge branch 'master' into new
>
> 	# ------------------------ >8 ------------------------
> 	# Do not modify or remove the line above.
> 	# Everything below it will be ignored.
> 	#
> 	# Conflicts:
> 	#	a

I have a mixed feeling about this change and I certainly would not
call it a "fix".

The reason why we give the list of conflicted paths that is
commented out is to remind the user of them in order to help them
describe what area of the codebase had overlapping changes, why, and
how the overlap was resolved, which is relevant information when
somebody else later needs to dig into the history to understand how
the code came into today's shape and why.  For that reason, if a
careless user left conflicts list behind without describing these
details about the merge, it might be better to have the unexplained
list in the merge than nothing.

In theory, the above argument applies the same way for the paths to
be committed, but the list is fairly trivial to recreate with "git
diff $it^!", unlike "which paths had conflict", which can only be
found out by recreating the auto-merge.  So in practice, the paths
that had conflicts is more worth showing than the paths that were
modified.

So, I dunno.  If we value the "more expensive list to reproduce",
the fix might be not to place it (and possibly the comments and
everything under the scissors line) behind a "# " comment char on
the line, without moving its position.

.

Denton Liu Nov. 14, 2018, 8:10 a.m. UTC | #2

On Wed, Nov 14, 2018 at 04:52:59PM +0900, Junio C Hamano wrote:
> Denton Liu <[e-mail]> writes:
> 
> > With this fix, the message becomes the following:
> >
> > 	Merge branch 'master' into new
> >
> > 	# ------------------------ >8 ------------------------
> > 	# Do not modify or remove the line above.
> > 	# Everything below it will be ignored.
> > 	#
> > 	# Conflicts:
> > 	#	a
> 
> I have a mixed feeling about this change and I certainly would not
> call it a "fix".
> 
> The reason why we give the list of conflicted paths that is
> commented out is to remind the user of them in order to help them
> describe what area of the codebase had overlapping changes, why, and
> how the overlap was resolved, which is relevant information when
> somebody else later needs to dig into the history to understand how
> the code came into today's shape and why.  For that reason, if a
> careless user left conflicts list behind without describing these
> details about the merge, it might be better to have the unexplained
> list in the merge than nothing.
> 

The reason why I implemented it this way is because the default
cleanup setting (strip) produces this message:

	Merge branch 'master' into new

	# Conflicts:
	#	a
	#
	# It looks like you may be committing a merge.
	# If this is not correct, please remove the file
	#	.git/MERGE_HEAD
	# and try again.


	# Please enter the commit message for your changes. Lines starting
	# with '#' will be ignored, and an empty message aborts the commit.
	#
	# On branch new
	# All conflicts fixed but you are still merging.
	#
	# Changes to be committed:
	#	modified:   a
	#

Which makes it seem like the `Conflicts:` section should be removed by
default.

> In theory, the above argument applies the same way for the paths to
> be committed, but the list is fairly trivial to recreate with "git
> diff $it^!", unlike "which paths had conflict", which can only be
> found out by recreating the auto-merge.  So in practice, the paths
> that had conflicts is more worth showing than the paths that were
> modified.
> 
> So, I dunno.  If we value the "more expensive list to reproduce",
> the fix might be not to place it (and possibly the comments and
> everything under the scissors line) behind a "# " comment char on
> the line, without moving its position.

If I understood correctly, then I have no strong opinions between
uncommenting the Conflicts section by default and this change; I just
think it'd be nice to have behaviour that's consistent.

> 
> .
> 
>