Re: [PATCH 11/14] replay: use standard revision ranges

From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Derrick Stolee <derrickstolee@github.com>,
	Christian Couder <christian.couder@gmail.com>,
	git@vger.kernel.org, Patrick Steinhardt <ps@pks.im>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	John Cai <johncai86@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 11/14] replay: use standard revision ranges
Date: Mon, 17 Apr 2023 22:58:11 -0700	[thread overview]
Message-ID: <CABPp-BEXfFWZ-0u4N1gU4L8o89FdJgBj_MaufzP7yGyi4GCSow@mail.gmail.com> (raw)
In-Reply-To: <xmqqy1mqo6kv.fsf@gitster.g>

On Mon, Apr 17, 2023 at 8:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> For instance, can users pass multiple ranges?
> >
> > There's no such thing as multiple ranges for most commands (see
> > f302c1e4aa0 ("revisions(7): clarify that most commands take a single
> > revision range", 2021-05-18))
> >
> > However, users should absolutely be allowed to specify something like
> >
> >   $ git replay --onto new-base master..my-topic some-base..other-topic
> >
> > which is one revision range, and I'd expect replay to take commits in
> > the history of either my-topic or other-topic, which are not in the
> > history of either master or some-base, and replay them.
>
> It is one revision range "^master ^some-base my-topic other-topic"
> if we let traditional revision parser to work on them, and in many
> topologies, it would mean something unexpected for users who thought
> that the user gave two ranges.  E.g. some-base may be an ancestor of
> master, in which case commits in the "some-base..master" range is
> not included in the result.  Or some-base may be able to reach
> my-topic, in which case no commits from master..my-topic range
> appears in the result, etc.
>
> But should we be forever limited by the "A..B is always equivalent
> to ^A B"?
>
> Shouldn't replaying master..my-topic and some-base..other-topic,
> when some-base is an ancestor of master, replay two overlapping
> range, possibly recreating some commits twice (when some-base falls
> in between master..my-topic, for example), if the user is willing to
> accept the consequences?
>
> We can still keep the "so called 'range' is just commits that are
> reachable from a positive end and are not reachable from any
> negative end, and by definition there is no such thing as multiple
> ranges" as an option for advanced users and trigger the semantics
> when one negative end is written explicitly with ^A notation, but in
> applications like cherry-pick where it is convenient to work on
> multiple ranges, we may want to extend our worldview to allow A..B
> C..D (especially when these two are distinct ranges---imagine the
> case where B is an ancestor of C) to mean "we have two ranges, and
> we mean the union of the commits in these two ranges".

Ooh, interesting.

That could also be of use in rebase-like operations (i.e. it'd serve
as a way for the user to rebase while dropping the commits between B
and C).

One thing to be careful about is that I think we would also need
parent rewriting or some other hint that B was an ancestor of C for
this to correctly work.  Otherwise, this construction would just be
mistaken for separate branches that are both being replayed.  That may
even matter for the cherry-pick case, since cherry-picking merge
commits is part of the intended scope, preventing us from just
assuming a fully linear range of commits to handle.

> We'd need to design carefully what should happen for trickier cases
> like A..B C (does it mean the traditional "a single range of commits
> that are reachable from either B or C, but never reachable from A",
> or does it mean "the union of commits A..B that are reachable from B
> but not from A and commits C that are reachable from C all the way
> down to root"?), though.

Yes, and in things like "^D A..B C", we'd have to not only worry about
whether A limits C, but also whether ^D limits A..B.

Thanks for the interesting food for thought.