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

[Phillip Wood <phillip.wood@talktalk.net>]

On 20/03/18 17:34, Junio C Hamano wrote:
> 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...
> 
>>> +		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.
> 
>>> +			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?
> 
I've not tested it but I think it's OK, I agree that it is clearer than
my version

Best Wishes

Phillip