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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 22, 2017, 16:03 UTC
Message-ID: <xmqq60df4mjy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <36bea69b-010a-9542-ec87-38b00d2bb009@grubix.eu>

Michael J Gruber <michael@grubix.eu> writes:

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

I was sort of expecting that tracing the control flow would give us the definite answer and that would be much better than any amount of testing.

In any case, I wasn't even suggesting to remove "if". It might even be worth doing

	if (squash)
		BUG("the control must not reach here under --squash");
	write_emrge_heads(...);

if we know the control does not have to reach with "--squash" in today's code, so that future careless refactoring does not break this fix.