Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2020, 19:55 UTC
Message-ID: <xmqqd099fnfm.fsf@gitster.c.googlers.com>
In-Reply-To: <20200318192821.43808-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 12 quoted lines

>> Hmph, what was it called earlier?  My gut reaction without much
>> thinking finds --no-skip-* a bit confusing double-negation and
>> suspect "--[no-]detect-cherry-pick" (which defaults to true for
>> backward compatibility) may feel more natural, but I suspect (I do
>> not recall details of the discussion on v1) it has been already
>> discussed and people found --no-skip-* is OK (in which case I won't
>> object)?
>
> It was earlier called "--{,no-}skip-already-present" (with the opposite
> meaning, and thus, --skip-already-present is the default), so the double
> negative has always existed. "--detect-cherry-pick" might be a better
> idea...I'll wait to see if anybody else has an opinion.

While "--[no-]detect-cherry-pick" is much better in avoiding double negation, it is a horrible name---we do not tell the users what we do after we detect cherry pick ("--[no-]skip-cherry-pick-detection" does not tell us, either).

Compared to them, "--[no-]skip-already-present" is much better, even though there is double negation.

How about a name along the lines of "--[no-]keep-duplicate", then?

Show 13 quoted lines

>> I also wonder if --detect-cherry-pick=(yes|no|auto) may give a
>> better end-user experience, with "auto" meaning "do run patch-ID
>> based filtering, but if we know it will be expensive (e.g. the
>> repository is sparsely cloned), please skip it".  That way, there
>> may appear other reasons that makes patch-ID computation expensive
>> now or in the fiture, and the users are automatically covered.
>
> It might be better to have predictability, and for "auto", I don't know
> if we can have a simple and explainable set of rules as to when to use
> patch-ID-based filtering - for example, in a partial clone with no
> blobs, I would normally want no patch-ID-based filtering, but in a
> partial clone with only a blob size limit, I probably will still want
> patch-ID-based filtering.

Perhaps. You could have something more specific than "auto". The main point was instead of "--[no-]$knob", "--$knob=(yes|no|...)" is much easier to extend. I simply do not know if we will see need to extend the vocabulary in the near future (to which you guys who are more interested in sparse clones would have much better insight than I do).

Thanks.