[v3,0/1] Fix scissors bug during merge conflict

Message

Denton Liu Nov. 17, 2018, 11:32 p.m. UTC

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(-)

Comments

Junio C Hamano Nov. 18, 2018, 6:54 a.m. UTC | #1

Denton Liu <[e-mail]> writes:

>> 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.

The original calls add-cut-line in the "whence != FROM_COMMIT" when
cleanup_mode is CLEANUP_SCISSORS (and then in that block it also adds
the message about committing a merge or cherry-pick).  After that,
the original does the three-arm if/else if/else cascade and we end
up showing the "lines starting with # will be kept" message.

Yeah, you read the original correctly and I agree that in that block
the right thing to do is not to say anything in CLEANUP_SCISSORS
mode.

Thanks for carefully reading the code again.