Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2018, 17:34 UTC
Message-ID: <xmqqfu4uslar.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1803201630380.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +		if (!keep_empty && is_empty)
>>  			strbuf_addf(&buf, "%c ", comment_line_char);

We are not trying to preserve an empty one, and have found an empty one, so we comment it out, and then...

Show 7 quoted lines

>> +		if (is_empty || !(commit->object.flags & PATCHSAME)) {
>
> May I suggest inverting the logic here, to make the code more obvious and
> also to avoid indenting the block even further?
>
> 		if (!is_empty && (commit->object.flags & PATCHSAME))
> 			continue;

... if a non-empty one that already appears in the upstream, we do not do anything to it. There is no room for keep-empty or lack of it to affect what happens to these commits.

Otherwise the insn is emitted for the commit.

Show 6 quoted lines

>> +			strbuf_addf(&buf, "%s %s ", insn,
>> +				    oid_to_hex(&commit->object.oid));
>> +			pretty_print_commit(&pp, commit, &buf);
>> +			strbuf_addch(&buf, '\n');
>> +			fputs(buf.buf, out);
>> +		}

I tend to agree that the suggested structure is easier to follow than Phillip's version.

But I wonder if this is even easier to follow. It makes it even more clear that patchsame commits that are not empty are discarded unconditionally.

	while ((commit = get_revision(&revs))) {
		int is_empty  = is_original_commit_empty(commit);
		if (!is_empty && (commit->object.flags & PATCHSAME))
			continue;
		strbuf_reset(&buf);
		if (!keep_empty && is_empty)
			strbuf_addf(&buf, "%c ", comment_line_char);
		strbuf_addf(&buf, "%s %s ", insn,
			    oid_to_hex(&commit->object.oid));
		pretty_print_commit(&pp, commit, &buf);
		strbuf_addch(&buf, '\n');
		fputs(buf.buf, out);
	}

Or did I screw up the rewrite?