Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 7, 2018, 23:25 UTC
Message-ID: <20180807232528.182492-1-jonathantanmy@google.com>
In-Reply-To: <xmqq7el1kdlt.fsf@gitster-ct.c.googlers.com>

Show 23 quoted lines

> Just a note (and a request-for-sanity-check) and not meant to be a
> request to update the code, but with a still-in-pu 4b757a40 ("Use
> remote_odb_get_direct() and has_remote_odb()", 2018-08-02) in
> flight, repository_format_partial_clone is now gone.
> 
> I've tentatively resolved the above to read like so:
> 
> 		if (has_remote_odb())
> 			repack_promisor_objects(&po_args, &names);
> 
> but I am not sure if it makes sense to always require odb helper to
> be present for any promisor.  As long as you do not have need to
> actually access these missing objects, you do not need any remote
> odb access at all, in which case requiring has_remote_odb() as a
> precondition to concatenate the promisor packs to coalesce them into
> one pack does not make sense---you only want to know if there are
> any .promisor packs.
> 
> In other words, I suspect that the world is not black (i.e. partial
> clone, which always has remote-odb) and white (i.e. full repository,
> without remote-odb).  4b757a40 makes it impossible to have a gray
> (i.e. partial clone, but no access to remote-odb), which I am not
> sure if it is a good thing.

Thanks for the heads-up. This makes me realize that even the current precondition (repository_format_partial_clone) is not an exact one - I should only be doing this if I know that there is at least one promisor object (if not, in the rare case that a repo is configured to be partial but does not have any promisor objects, repack will generate an empty packfile). In the next reroll, I'll take care of this, which should avoid this merge issue too.