On Sat, Nov 17, 2018 at 05:06:43PM +0900, Junio C Hamano wrote:
> Are we already sometimes adding a scissors line in that file? The
> impression I was getting was that the change in the step 2/2 is the
> only reason why this becomes necessary. In which case this change
> makes no sense as a standalone patch and requires 2/2 to be a
> sensible change, no?
>
My mistake, I guess I went a little overboard trying to split my
contribution into digestable patches.
> > @@ -798,7 +807,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 &&
> > + !merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > status_printf_ln(s, GIT_COLOR_NORMAL,
> > whence == FROM_MERGE
>
> This one is done before we show a block of text, which looks correct.
>
> > @@ -824,7 +834,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 &&
> > + !merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> The correctness of this one in an if/elseif/else cascade is less
> clear. When "contains scissors" logic does not kick in, we just
> have a scissors line here (i.e. the original behaviour). Now when
> the new logic kicks in, we not just omit the (extra) scissors line,
> but also say "Please enter the commit message..." which is the
> message used with the "comment line char" mode of the clean-up.
>
> I wonder if this is what you really meant to have instead:
>
> > else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > - whence == FROM_COMMIT)
> > - wt_status_add_cut_line(s->fp);
> > + whence == FROM_COMMIT) {
> > + if (!merge_contains_scissors)
> > + wt_status_add_cut_line(s->fp);
> > + }
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> That is, the only behaviour change in "merge contains scissors"
> mode is to omit the cut line and nothing else.
Thanks for catching this. I noticed that the originally behaviour is
buggy too: in the case where we're merging a commit and scissors are
printed from the `if (whence != FROM_COMMIT)` block, the original
behaviour would drop us into the else (COMMIT_MSG_CLEANUP_SPACE)
statement, which is undesired. With this fix, the message about `#`
_not_ being removed is now silenced in both cases.
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
Denton Liu (1):
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 | 48 +++++++++++++++++++++++++++++++++
5 files changed, 91 insertions(+), 5 deletions(-)
--
2.19.1
