On Tue, Nov 01, 2022 at 08:20:48AM +0100, Patrick Steinhardt wrote:
> On Mon, Oct 31, 2022 at 09:28:09PM -0400, Taylor Blau wrote:
> > On Mon, Oct 31, 2022 at 03:45:47PM +0100, Patrick Steinhardt wrote:
> > > On Sun, Oct 30, 2022 at 03:09:04PM -0400, Taylor Blau wrote:
> > > > On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote:
> > > > > This strategy has the major downside that it will not require any object
> > > > > to be sent by the client that is reachable by any of the repositories'
> > > > > references. While that sounds like it would be indeed what we are after
> > > > > with the connectivity check, it is arguably not. The administrator that
> > > > > manages the server-side Git repository may have configured certain refs
> > > > > to be hidden during the reference advertisement via `transfer.hideRefs`
> > > > > or `receivepack.hideRefs`. Whatever the reason, the result is that the
> > > > > client shouldn't expect that any of those hidden references exists on
> > > > > the remote side, and neither should they assume any of the pointed-to
> > > > > objects to exist except if referenced by any visible reference. But
> > > > > because we treat _all_ local refs as uninteresting in the connectivity
> > > > > check, a client is free to send a packfile that references objects that
> > > > > are only reachable via a hidden reference on the server-side, and we
> > > > > will gladly accept it.
> > > >
> > > > You mention below that this is a correctness issue, but I am not sure
> > > > that I agree.
> > > >
> > > > The existing behavior is a little strange, I agree, but your argument
> > > > relies on an assumption that the history on hidden refs is not part of
> > > > the reachable set, which is not the case. Any part of the repository
> > > > that is reachable from _any_ reference, hidden or not, is reachable by
> > > > definition.
> > > >
> > > > So it's perfectly fine to consider objects on hidden refs to be in the
> > > > uninteresting set, because they are reachable. It's odd from the
> > > > client's perspective, but I do not see a path to repository corruption
> > > > with thee existing behavior.
> > >
> > > Indeed, I'm not trying to say that this can lead to repository
> > > corruption.
> > 
> > I definitely agree with that. I have thought about this on-and-off since
> > you sent the topic, and I am pretty certain that there is no path to
> > repository corruption with the existing behavior. It would be worth
> > updating the commit message to make this clearer.
> 
> Fair enough, I can try to do that.
> 
> > > But security-related or not, I think it is safe to say that any packfile
> > > sent by a client that does not contain objects required for the updated
> > > reference that the client cannot know to exist on the server-side must
> > > be generated by buggy code.
> > 
> > Maybe, though I think it's fine to let clients send us smaller packfiles
> > if they have some a-priori knowledge that the server has objects that it
> > isn't advertising. And that can all happen without buggy code. So it's
> > weird, but there isn't anything wrong with letting it happen.
> 
> Well, I don't see how to achieve both at the same time though: we can
> either limit the set of uninteresting tips to what we have announced to
> the client, or we allow clients to omit objects that have not been
> announced. These are mutually exclusive.
> 
> So if we take the stance that it was fine to send packfiles that omit
> hidden objects and that this is something we want to continue to support
> then this patch series probably becomes moot. Doing the proposed
> optimization means that we also tighten the rules here.

I'm wrong and you're right: we can do the optimization to limit the refs
we use but still let clients send objects that are hidden. I didn't take
into account that this is merely an optimization that we stop walking at
reachable tips. I'll reword the commit message when having another go
and will likely do something along the lines of your proposed new
`--visible-refs` option in v2 of this series.

Patrick