Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 9, 2018, 09:27 UTC
Message-ID: <xmqqtvlvpj0m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181008214816.42856-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> Because cache_tree_update() is used from multiple places, this new
> behavior is guarded by a new flag WRITE_TREE_SKIP_WORKTREE_MISSING_OK.

The name of the new flag is mouthful, but we know we do not need to materialize these blobs (exactly because the skip-worktree bit is set, so we do not need to know what's in these blobs) and it is OK for these to be missing (to put it differently, we do not care if they exist or not---hence we short-circuit the otherwise required call to has_object_file()), iow, the name of the mode is "A missing object with skip-worktree bit set is OK", which makes sense to me.

Show 5 quoted lines

>  		if (is_null_oid(oid) ||
> -		    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {
> +		    (mode != S_IFGITLINK && !missing_ok &&
> +		     !(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&
> +		     !has_object_file(oid))) {

For a non-gitlink entry, if the caller does not say "any missing object is OK", we normally check has_object_file(). But now has_object_file() call happens only when ...

Hmph, isn't this new condition somewhat wrong? We do not want to skip it for entries without skip-worktree bit set. We only do not care if we are operating in skip-worktree-missing-ok mode and the bit is set on ce. IOW:

	if ((skip_worktree_missing_ok && ce_skip_worktree(ce)) ||
            has_object_file(oid))
		/* then we are happy */

but the whole thing above tries to catch problematic case, so I'd need to negate that, i.e.

	if ( ... &&
	    !((skip_worktree_missing_ok && ce_skip_worktree(ce)) ||
             has_object_file(oid)))
		/* we are in trouble */

and pushing the negation down, we get

	if ( ... &&
	    (!(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&
             !has_object_file(oid)))
		/* we are in trouble */

OK. The logic in the patch is correct. I however feel that it is a bit too dense to make sense of.

Thanks, will queue.