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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 20 Mar 2018, 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?

This looks correct. And the postimage is easier to follow than the one of
my suggested change.

My version is easier to review on the mailing list, of course, as it
minimizes the diff... ;-)

Ciao,
Dscho