Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index

From: Elijah Newren <newren@gmail.com>
Date: Jun 7, 2018, 07:41 UTC
Message-ID: <CABPp-BFzOkX_uSyiVH-S8mqxeOin0xcYR-EzXUtvcdN7dGhidQ@mail.gmail.com>
In-Reply-To: <20180606165016.3285-5-pclouds@gmail.com>

Subject line: unpack-trees rather than unpack-tress.

On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 18 quoted lines

> Prior to fba92be8f7, this code implicitly (and incorrectly) assumes
> the_index when running the exclude machinery. fba92be8f7 helps show
> this problem clearer because unpack-trees operation is supposed to
> work on whatever index the caller specifies... not specifically
> the_index.
>
> Update the code to use "istate" argument that's originally from
> mark_new_skip_worktree(). From the call sites, both in unpack_trees(),
> you can see that this function works on two separate indexes:
> o->src_index and o->result. The second mark_new_skip_worktree() so far
> has incorecctly applied exclude rules on o->src_index instead of
> o->result. It's unclear what is the consequences of this, but it's
> definitely wrong.
>
> [1] fba92be8f7 (dir: convert is_excluded_from_list to take an index -
>     2017-05-05)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

A somewhat curious finding: when I was rebuilding and re-testing all 23 patches, I got a failure on this patch in test 31 of t7063-status-untracked-cache.sh. I did not get any test failures with any of the other patches. However, after re-running that test or the whole suite half a dozen times with just up to this patch applied, I was not able to trigger the failure again. Is there a rare race in that testcase? I certainly don't see anything in this patch that appears problematic, and the fact that I couldn't readily reproduce suggests it could well have been there before any of these patches.

Everything else in the patch looks fine to me.