Re: [PATCH 0/3] rev-list: add support for commits in `--missing`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

On Thu, Oct 12, 2023 at 12:44:27PM +0200, Karthik Nayak wrote:
> On Wed, Oct 11, 2023 at 6:54 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Karthik Nayak <karthik.188@gmail.com> writes:
> >
> > > Seems like this is because of commit-graph being enabled, I think
> > > the best thing to do here would be to disable the commit graph of
> > > these tests.
> >
> > If the CI uncovered that the new code is broken and does not work
> > with commit-graph, wouldn't the above a totally wrong approach to
> > correct it?  If the updated logic cannot work correctly when
> > commit-graph covers the history you intentionally break, shouldn't
> > the code, when the feature that is incompatible with commit-graph is
> > triggered, disable the commit-graph?  I am assuming that the new
> > feature is meant to be used to recover from a corrupt repository,
> > and if it does not work well when commit-graph knows (now stale
> > after repository corruption) more about the objects that are corrupt
> > in the object store, we do want to disable commit-graph.  After all,
> > commit-graph is a secondary information that is supposed to be
> > recoverable from the primary data that is what is in the object
> > store.  Disabling commit-graph in the test means you are telling the
> > end-users "do not use commit-graph if you want to use this feature",
> > which sounds like a wrong thing to do.
> 
> I agree with what you're saying. Disabling writing the commit-graph for
> only the test doesn't serve the real usage. To ensure that the feature should
> work with corrupt repositories with stale commit-graph, I'm thinking of
> disabling the commit-graph whenever the `--missing` option is used. The
> commit graph already provides an API for this, so it should be simple to do.
> 
> Thanks!

Wouldn't this have the potential to significantly regress performance
for all those preexisting users of the `--missing` option? The commit
graph is quite an important optimization nowadays, and especially in
commands where we potentially walk a lot of commits (like we may do
here) it can result in queries that are orders of magnitudes faster.

If we were introducing a new option then I think this would be an
acceptable tradeoff as we could still fix the performance issue in
another iteration. But we don't and instead aim to change the meaning of
`--missing` which may already have users out there. Seen in that light
it does not seem sensible to me to just disable the graph for them.

Did you figure out what exactly the root cause of this functional
regression is? If so, can we address that root cause instead?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]