Re: [PATCH] fetch: when deepening, check connectivity fully

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 29, 2018, 22:30 UTC
Message-ID: <20180629223024.62002-1-jonathantanmy@google.com>
In-Reply-To: <xmqqwoujdejl.fsf@gitster-ct.c.googlers.com>

Show 11 quoted lines

> > That is the way it should work, but after thinking about it once more, I
> > realize that it isn't.
> >
> > opt->shallow_file is not set to anything. And fetch-pack updates the
> > shallow file by itself (at least, that is my understanding of
> > update_shallow() in fetch-pack.c) before fetch calls check_connected(),
> > meaning that if check_connected() fails, there is still no rollback of
> > the shallow file.
> 
> Ouch.  We need to fix that; otherwise, a broken server will keep
> giving you a corrupt repository even with this fix, no?

Yes, that is true - the repository will be left in a corrupt state.

I did some more investigation, and usually things are OK because unpack-trees runs a fsck_walk on all the objects it unpacks (with --shallow-file appropriately set). Things are bad only if the packfile is empty (or, presumably, if the packfile only has unrelated objects).

I managed to use the one-time-sed mechanism to craft a response that triggers this case. This both enables me to avoid the brittle check of "rev-list" appearing in the GIT_TRACE output (as discussed in [1]), and to verify that a rollback of the shallow file works, once such a patch is written. I'll look into writing such a patch, so feel free to hold off on this until that patch is done.

[1] https://public-inbox.org/git/20180627225105.155996-1-jonathantanmy@google.com/