Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument

From: Jonathan Tan <jonathantanmy@google.com>
To: Elijah Newren <newren@gmail.com>
Cc: Jonathan Tan <jonathantanmy@google.com>,
	Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument
Date: Fri, 19 Aug 2022 10:25:19 -0700	[thread overview]
Message-ID: <20220819172519.3703282-1-jonathantanmy@google.com> (raw)
In-Reply-To: <CABPp-BFjxFeGO+NU4HFCGqDe4aRFhqOdOxNYVDf7EJOWdT5RgA@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:
> > Optional: Besides that, from what I can tell, sometimes the C commits
> > themselves are marked with ANCESTRY_PATH (when they are explicitly
> > specified) and sometimes they are not (when they are not explicitly
> > specified). It's not a bug here, but it might be worth handling that in
> > the ancestry_path_need_bottoms codepath (instead of explicitly setting
> > TMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we
> > can also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry
> > path codepaths, but I haven't tested it), at least to prevent possible
> > future bugs.
> 
> That sounds like you're trying to duplicate the bug in my first
> attempt at this patch.  If you try to coalesce ANCESTRY_PATH and
> TMP_MARK, then you not only get all descendants of C, you also get all
> descendants of any ancestor of C, which defeats the whole point of my
> changes.

Ah, yes you're right.

> It's true that I don't mark implicit C commits with ANCESTRY_PATH, but
> those are always bottom commits that are the excluded end of a range
> anyway.  While those could be marked without causing problems, it
> would always be a waste of effort.

Yes, that's true.