Re: [PATCH v3 4/4] connected: add incremental connectivity check via rev-list

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 16, 2026 at 01:50:21PM +0200, Kristofer Karlsson wrote:
> On Thu, 15 Oct 2026 at 13:50, Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > I think this is where it gets tricky to reason about.  The win
> > > from from larger trees pays off without bound -- the larger
> > > trees you have, the larger the win.
> > >
> > > Scaling with commits on the other hand we have ceiling of how
> > > much it can regress.  I am thinking that could be reasonable
> > > that anyone who enables the config flag for this mode could
> > > simply accept that it would likely speed things up in most
> > > cases but in other cases they could see a ~1.5x regression.
> > >
> > > I am hoping that this tradeoff would then be acceptable,
> > > otherwise we would indeed need to add some way of switching,
> > > though I don't think there's a single good indicator value
> > > for when it's better to switch.
> > >
> > > I could add a new config flag like
> > > transfer.connectivityCheckIncrementalMaxCommits to control it
> > > perhaps.
> >
> > Yeah, making this configurable may be reasonable. The way I understand
> > it, the new connectivity check is most likely to be usable for smaller,
> > incremental pushes. And based on the above table you have, we see that
> > up to 1k commits the new algorithm is faster. Sure, it's an artificial
> > benchmark, but maybe this is a good enough cutoff for now?
> 
> I looked into this a bit more and I have two main notes:
> 
> First of all, I benchmarked multiple repos, both with small
> object trees (such as git.git), medium (linux.git) and some
> really big ones (proprietary).
> 
>     repo          files   commits   full    incr.  full/incr
>     internal A    552K       1000   1.69s   0.66s    2.6x
>     internal B    331K       1000   1.48s   0.57s    2.6x
>     linux          95K        609   0.34s   0.12s    2.8x
>     linux          95K      13965   1.86s   1.90s    1.0x
>     git.git        4.9K       356   0.09s   0.13s    0.7x
>     git.git        4.9K       976   0.23s   0.31s    0.7x
> 
> So for large and medium repos the cutoff of 1000 is
> conservative. Incremental mode is faster but the full mode
> is also good enough here.
> 
> For small repos like git, the incremental mode is slightly
> slower, but since the number of files is so small, both modes
> are still fast enough that it does not matter.
> 
> (I could not get an exact 1000 commit test synthetically by
> looking back in the history for all repos, since it depends
> a bit on the shape of the graph of commits.)
> 
> Thus, I think having a cutoff of 1000 is indeed a valid
> strategy here.

Yeah, thanks for evaluating this. Heuristics are of course going to go
off rails at times, but I agree that 1000 seems like a good enough first
iteration.

> Second of all (and with the previous in mind), I think I would
> want to change the configuration into three modes:
> 
>   - full (default): work as it does today.
>   - incremental:    only use the incremental mode
>   - auto:           decide on full or incremental depending
>                     number of commits to process.
> 
> This would make things safe to roll out and start gaining more
> real life confidence before potentially changing the default
> to auto instead.
> 
> We could also make the cutoff configurable, but I am not sure
> that is needed in practice.

Hmm. As the heuristic can easily go wrong I think it would be a good
idea to make it configurable. So I'd propose `auto[=<cutoff>]`. If left
out we default to 1000, otherwise we pick whatever the user has
specified.

[snip]
> > > > > +     if (type >= 0)
> > > > > +             die(_("object %s is a %s, not a %s"),
> > > > > +                 oid_to_hex(oid), type_name(type), "blob");
> > > >
> > > > I'd love not having all these calls to `die()`, as this logic is
> > > > essentially part of a library, and dying inside a library is a bad
> > > > smell. We do this everywhere else in Git, too, but we're slowly getting
> > > > rid of this as part of our libification efforts.
> > >
> > > I think this would be a bigger refactor in general --
> > > the existing connectivity check also dies as the way to
> > > report problems.  This works since it's running as a subprocess.
> > >
> > > If this is something we want to change, I think it would be
> > > good to do that refactoring separately and make the connectivity
> > > check subprocess only die() at the very edge, and making all of
> > > the internal library code support not dying.  My first prototype
> > > tried to do this but the amount of changes to internal code got
> > > out of hand, so this version was the simplest I could make it.
> > >
> > > But I'm happy to take a step back and revisit that approach if
> > > this is a blocker.
> >
> > I mostly aim at not making _new_ library code die anymore. In any case,
> > I don't think it's a blocker, but I nowadays consider this to be a code
> > smell.
> 
> I have spent some more time thinking about this and I have
> changed my mind -- I think I was too stuck in the idea of
> all-or-nothing here, but I agree I can make this work in a
> mixed way -- not introducing any new die() but accept the
> cases where it already dies.
> 
> I will include this in the reroll (likely next week, don't
> want to rush things).

Thanks, I appreciate it when folks take their time :)

Patrick