[PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
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 --- unpack-trees.c | 31 ++- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 45fcda3169..5268de7af5 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str return mask; } -static int clear_ce_flags_1(struct cache_entry **cache, int nr, +static int clear_ce_flags_1(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, struct exclude_list *el, int defval); /* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask, @@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr, struct cache_entry **cache_end; int dtype = DT_DIR; int ret = is_excluded_from_list(prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate); int rc; strbuf_addch(prefix, '/'); @@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr, * calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc = clear_ce_flags_1(cache, cache_end - cache, + rc = clear_ce_flags_1(istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret); @@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr, * cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1(struct cache_entry **cache, int nr, +static int clear_ce_flags_1(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, struct exclude_list *el, int defval) @@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name; strbuf_add(prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask, @@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, } strbuf_addch(prefix, '/'); - cache += clear_ce_flags_1(cache, cache_end - cache, + cache += clear_ce_flags_1(istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval); strbuf_setlen(prefix, prefix->len - len - 1); @@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, /* Non-directory */ dtype = ce_to_dtype(ce); ret = is_excluded_from_list(ce->name, ce_namelen(ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate); if (ret < 0) ret = defval; if (ret > 0) @@ -1213,
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
Subject line: unpack-trees rather than unpack-tress. On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy wrote: > 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 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.
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
On Thu, Jun 7, 2018 at 9:41 AM, Elijah Newren wrote: > Subject line: unpack-trees rather than unpack-tress. > > > > On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy > wrote: >> 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 > > 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? Untracked cache tests are very time-sensitive. I'll try to run and re-run them a couple times to understand more. Thanks for pointing it out. -- Duy
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
On Thu, Jun 7, 2018 at 5:11 PM Duy Nguyen wrote: > > On Thu, Jun 7, 2018 at 9:41 AM, Elijah Newren wrote: > > Subject line: unpack-trees rather than unpack-tress. > > > > > > > > On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy > > wrote: > >> 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 > > > > 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? > > Untracked cache tests are very time-sensitive. I'll try to run and > re-run them a couple times to understand more. Thanks for pointing it > out. after hours of running tests, either with full 23 patches or just the first 4, and failing to catch the failure, i declare (or more like, pray) that you ran into a crack of time that led to a race. I'll take no action on this, but I'll remember this and watch out for untracked cache related mails for some time. -- Duy
