Re: [PATCH v2 3/3] merge: save merge state earlier

From: Michael J Gruber <michael@grubix.eu>
Date: Aug 22, 2017, 09:36 UTC
Message-ID: <36bea69b-010a-9542-ec87-38b00d2bb009@grubix.eu>
In-Reply-To: <xmqqk21w4et2.fsf@gitster.mtv.corp.google.com>

Junio C Hamano venit, vidit, dixit 22.08.2017 02:38:

Show 21 quoted lines

> Michael J Gruber <git@grubix.eu> writes:
> 
>>  static void prepare_to_commit(struct commit_list *remoteheads)
>>  {
>>  	struct strbuf msg = STRBUF_INIT;
>> @@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>>  		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
>>  	if (signoff)
>>  		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>> +	if (!squash)
>> +		write_merge_heads(remoteheads);
>>  	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
>>  	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>>  			    git_path_merge_msg(), "merge", NULL))
> 
> I can understand that you would never want to write out MERGE_HEAD
> while squashing, but I somehow think it would be a bug in the caller
> to call prepare_to_commit(), whose point is to prepare the merge
> message to be recorded in the resulting merge commit, when the user
> gave us the "--squash" option, which is an explicit instruction that
> the user does not want the merge commit the message is used.

That sounds reasonable. I vaguely remember a failing test for an earlier version that I tried, but that was before the "split".

Show 11 quoted lines

> Can squash ever be true in this function?
> 
> This function has two callsites: merge_trivial() and
> finish_automerge().
> 
> I think merge_trivial() will not be called under "--squash", which
> turns option_commit off and the only callsite of it is inside an
> else-if clause that requres option_commit to be true.  You can do a
> similar deduction around the "automerge_was_ok" variable to see if
> finish_automerge() can be called when "--squash" is given; I suspect
> the answer may be no.

I'll go without the if, after more testing.

Show 23 quoted lines

>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
>> index 2ebda509ac..80194b79f9 100755
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with --continue' '
>>  	verify_parents $c0 $c1
>>  '
>>  
>> +write_script .git/FAKE_EDITOR <<EOF
>> +# kill -TERM command added below.
>> +EOF
>> +
>> +test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue' '
>> +	git reset --hard c0 &&
>> +	! "$SHELL_PATH" -c '\''
>> +	  echo kill -TERM $$ >> .git/FAKE_EDITOR
>> +	  GIT_EDITOR=.git/FAKE_EDITOR
>> +	  export GIT_EDITOR
>> +	  exec git merge --no-ff --edit c1'\'' &&
> 
> This is a tricky construct.  You "reserve" a process ID by using a
> shell, arrange it to be killed and then using "exec" to make it the
> "git merge" program to be killed.  I kind of like the convolutedness.

That is from t7502. Sorry for hiding that note in the cover letter, I should put it into 3/3's message or a test comment.

When testing, I simply used "git merge... &" and "ps" or "jobs" to know which process to kill. Apparantly, the above is the most portable way to script that. t7502 went through a few iterations to ensure this.

Show 5 quoted lines

>> +	git merge --continue &&
>> +	verify_parents $c0 $c1
>> +'
>> +
>>  test_done