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

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs
Date: Tue, 09 Oct 2018 18:27:21 +0900	[thread overview]
Message-ID: <xmqqtvlvpj0m.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20181008214816.42856-1-jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 8 Oct 2018 14:48:16 -0700")

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.

>  		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.