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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 27, 2018, 22:40 UTC
Message-ID: <20180627224033.150025-1-jonathantanmy@google.com>
In-Reply-To: <xmqqin64dmui.fsf@gitster-ct.c.googlers.com>

Show 9 quoted lines

> Hmph, remind me how old and/or new shallow cut-off points affect
> this traversal?  In order to verify that everything above the new
> cut-off points, opt->shallow_file should be pointing at the new
> cut-off points, then we do the full sweep (without --not --all) to
> ensure we won't find missing or broken objects but still stop at the
> new cut-off points, and then only after check_connected() passes,
> update the shallow file to make new shallow cut-off points active
> (and if the traversal fails, then we do nto install the new shallow
> cut-off points)?

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.

So to directly answer your first question, only the new shallow cut-off points affect this traversal, and not the old ones.

Show 9 quoted lines

> If so, that sounds sensible.  By not having "--not --all", we
> potentially would do a full sweep, but if we are really deepening to
> the root commits, then we do need one full sweep anyway (as these
> deepest parts of the history were previously hidden by the shallow
> cutoff points), and if we are only deepening the history by a bit,
> even if we do not have "--not --all", we would hit the updated
> shallow cutoff points (which may be at older parts of the history)
> and stop, and for correctness we need to visit there anyway, so I
> think we are not being overly pessimistic with this change, either.

Yes, this analysis makes sense.