Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
Denton Liu 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.
Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
On Wed, Nov 21, 2018 at 06:38:20PM +0900, Junio C Hamano wrote: > Denton Liu 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!
Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
Denton Liu 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.
[PATCH v4 0/2] Fix scissors bug during merge conflict
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(-) -- 2.19.1
