Re: [PATCH] fetch: in partial clone, check presence of targets

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2018, 23:28 UTC
Message-ID: <xmqqy3bvycie.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180920221038.74193-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> The hard part for me lies in how to communicate to future readers of the
> code that they cannot remove this section to simplify the code. We would
> need a more complicated comment, something like this:

That suggests two things.

 - Perhaps quickfetch() is misnamed.  It is to ensure "these exist,
   and are 'connected'"; renaming the helper to convey that would be
   sufficient to prevent future readers from removing the "these
   exist" check from it.

 - Perhaps check_connected() is also misnamed, if the above "these
   exist, and are 'connected'" is not a sufficient warning against
   removal of the "these exist" test, perhaps "check_connected()" is
   not telling the readers that things that are 'connected' do not
   have to exist.  What does being 'connected' mean in the world
   with "promised" objects anyway?  The designer of the feature
   should probably have a concise and clear answer.

>  /*
>   * Check if all wanted objects are present.

Here 'wanted' means... the tip that was directly requested must exist, and in addition, anything that is reachable from it must either exist locally or available from the lazy-clone source? But that is not quite it. Your definition of 'present' is fuzzy and mean two different things---for the wanted tips, they must exist. For the objects that are required for these wanted tips to be well formed, they do not have to exist but it is OK for them to be merely promised.

Perhaps the comment for the quickfetch() function itself should say

/*
 * Ensure that the requested tips exist locally, and anything that is
 * reachable from them either exist locally or promised to be available.
 */

Adding a similar comment to check_connected() function is left as an exercise, but I suspect it would be the latter half of the above sentence.

It may be worth renaming both functions for clarity, as I mentioned already.