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@xxxxxxxxx> wrote: > > > > Karthik Nayak <karthik.188@xxxxxxxxx> 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:
signature.asc
Description: PGP signature
