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

Message

Denton Liu Nov. 21, 2018, 3:13 a.m. UTC

Thanks for catching that, Szeder. I tested this revised patch under
GETTEXT_POISON and it should be passing tests now.

Changes since V1:
	* Only check MERGE_MSG for a scissors line instead of all prepended
	  files
	* Make a variable static in merge where appropriate
	* Add passthrough options in pull
	* Add documentation for the new option
	* Add tests to ensure desired behaviour

Changes since V2:
	* Merge both patches into one patch
	* Fix bug in help message printing logic

Changes since V3:
	* Add patch to cleanup 'merge --squash c3 with c7' test in t7600
	* Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests

Denton Liu (2):
  t7600: clean up 'merge --squash c3 with c7' test
  merge: add scissors line on merge conflict

 Documentation/merge-options.txt |  6 ++++
 builtin/commit.c                | 20 +++++++++----
 builtin/merge.c                 | 16 ++++++++++
 builtin/pull.c                  |  6 ++++
 t/t7600-merge.sh                | 53 ++++++++++++++++++++++++++++++---
 5 files changed, 92 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Nov. 21, 2018, 9:38 a.m. UTC | #1

Denton Liu <[e-mail]> writes:

> Changes since V3:
> 	* Add patch to cleanup 'merge --squash c3 with c7' test in t7600
> 	* Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests

Queued. Thanks, both.

Denton Liu Nov. 22, 2018, 1:10 a.m. UTC | #2

On Wed, Nov 21, 2018 at 06:38:20PM +0900, Junio C Hamano wrote:
> Denton Liu <[e-mail]> writes:
> 
> > Changes since V3:
> > 	* Add patch to cleanup 'merge --squash c3 with c7' test in t7600
> > 	* Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests
> 
> Queued. Thanks, both.

I just realised that there is a slight problem with the proposed change.
When we do a merge and there are no merge conflicts, at the end of the
merge, we get dropped into an editor with this text:

	Merge branch 'master' into new

	# Please enter a commit message to explain why this merge is necessary,
	# especially if it merges an updated upstream into a topic branch.
	#
	# Lines starting with '#' will be ignored, and an empty message aborts
	# the commit.

Note that in git-merge, the cleanup only removes commented lines and
this cannot be configured to be scissors or whatever else. I think that
the fact that it's not configurable isn't a problem; most hardcore
commit message editing happens in git-commit anyway.

However, since we taught git-merge the --cleanup option, this might be
misleading for the end-user since they would expect the MERGE_MSG to be
cleaned up as specified.

I see two resolutions for this. We can either rename --cleanup more
precisely so users won't be confused (perhaps something like
--merge-conflict-scissors but a lot more snappy) or we can actually make
git-merge respect the cleanup option and post-process the message
according to the specified cleanup rule.

I would personally think the first option is better than the second but
I'd like to hear your thoughts.

Thanks!

Junio C Hamano Nov. 24, 2018, 2:05 a.m. UTC | #3

Denton Liu <[e-mail]> writes:

> I just realised that there is a slight problem with the proposed change.
> When we do a merge and there are no merge conflicts, at the end of the
> merge, we get dropped into an editor with this text:
>
> 	Merge branch 'master' into new
>
> 	# Please enter a commit message to explain why this merge is necessary,
> 	# especially if it merges an updated upstream into a topic branch.
> 	#
> 	# Lines starting with '#' will be ignored, and an empty message aborts
> 	# the commit.
>
> Note that in git-merge, the cleanup only removes commented lines and
> this cannot be configured to be scissors or whatever else. I think that
> the fact that it's not configurable isn't a problem; most hardcore
> commit message editing happens in git-commit anyway.

OK.

> However, since we taught git-merge the --cleanup option, this might be
> misleading for the end-user since they would expect the MERGE_MSG to be
> cleaned up as specified.
>
> I see two resolutions for this. We can either rename --cleanup more
> precisely so users won't be confused (perhaps something like
> --merge-conflict-scissors but a lot more snappy) or we can actually make
> git-merge respect the cleanup option and post-process the message
> according to the specified cleanup rule.

The former certainly would be simpler to implement, but feels more
like an excuse for not doing the right thing to me, when I put
myself in shoes of users who use 'scissors' clean-up option in
commit.  I dunno.