On 20/03/18 17:34, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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
