On Mon, Jan 23, 2017 at 12:27 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes:
>> +static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)
>> {
>> int index_unchanged, empty_commit;
>>
>> /*
>> - * Three cases:
>> + * Four cases:
>> *
>> - * (1) we do not allow empty at all and error out.
>> + * (1) we do not allow empty at all and error out;
>> *
>> - * (2) we allow ones that were initially empty, but
>> + * (2) we skip empty commits altogether;
>> + *
>> + * (3) we allow ones that were initially empty, but
>> * forbid the ones that become empty;
>> *
>> - * (3) we allow both.
>> + * (4) we allow both.
>> */
>
> The original gave callers the choice to tell two cases (a commit was
> empty in the original history, and a commit that was not empty in
> the original history turns out to be redundant) apart and handle
> them differently. I tend to agree that skipping the former should
> be the norm, and also I think it is sensible to drop the latter, and
> that is what your updated (2) gives us, I think.
>
> But I would suspect that it would rather be common to have a
> deliberately empty commit in the original as a marker in a history
> and want to keep that across cherry-picking a series, while wanting
> to discard/skip patches that are already applied in an updated base.
> Shouldn't that be supported as the fifth case?
I was actually wondering about that. I guess the best approach
(symmetric wrt to the --allow) would be to intro introduce
--skip-empty _and_ --skip-redundant, with the former implying the
latter.
By the way, I noticed going over the code that the -allow options are
not stored, so that in case of interruption they will be reset, is
this intentional or a bug?
--
Giuseppe "Oblomov" Bilotta