| Message ID | fda23f07e6a20408b0a10c8944d54e7c65a1d629.1614111270.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: Design, Format, Tests | expand |
On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > The index_pos_by_traverse_info() currently throws a BUG() when a > directory entry exists exactly in the index. We need to consider that it > is possible to have a directory in a sparse index as long as that entry > is itself marked with the skip-worktree bit. > > The negation of the 'pos' variable must be conditioned to only when it > starts as negative. This is identical behavior as before when the index > is full. Same comment on the second paragraph as I made in the RFC series -- https://lore.kernel.org/git/CABPp-BGPJgA4guWHVm3AVS=hM0fTixUpRvJe5i9NnHT-3QJMfw@mail.gmail.com/. I apologize if I'm repeating stuff you chose to not change, but I didn't see a response and given the three typos left in previous patches, I'm unsure whether it was unaddressed on purpose or on accident. > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > unpack-trees.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index 4dd99219073a..b324eec2a5d1 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -746,9 +746,12 @@ static int index_pos_by_traverse_info(struct name_entry *names, > strbuf_make_traverse_path(&name, info, names->path, names->pathlen); > strbuf_addch(&name, '/'); > pos = index_name_pos(o->src_index, name.buf, name.len); > - if (pos >= 0) > - BUG("This is a directory and should not exist in index"); > - pos = -pos - 1; > + if (pos >= 0) { > + if (!o->src_index->sparse_index || > + !(o->src_index->cache[pos]->ce_flags & CE_SKIP_WORKTREE)) > + BUG("This is a directory and should not exist in index"); > + } else > + pos = -pos - 1; > if (pos >= o->src_index->cache_nr || > !starts_with(o->src_index->cache[pos]->name, name.buf) || > (pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf))) > -- > gitgitgadget >
On 2/25/2021 2:40 AM, Elijah Newren wrote: > On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <dstolee@microsoft.com> >> >> The index_pos_by_traverse_info() currently throws a BUG() when a >> directory entry exists exactly in the index. We need to consider that it >> is possible to have a directory in a sparse index as long as that entry >> is itself marked with the skip-worktree bit. >> >> The negation of the 'pos' variable must be conditioned to only when it >> starts as negative. This is identical behavior as before when the index >> is full. > > Same comment on the second paragraph as I made in the RFC series -- > https://lore.kernel.org/git/CABPp-BGPJgA4guWHVm3AVS=hM0fTixUpRvJe5i9NnHT-3QJMfw@mail.gmail.com/. > I apologize if I'm repeating stuff you chose to not change, but I > didn't see a response and given the three typos left in previous > patches, I'm unsure whether it was unaddressed on purpose or on > accident. Yes, I dropped this one. How about this? The 'pos' variable is assigned a negative value if an exact match is not found. Since a directory name can be an exact match, it is no longer an error to have a nonnegative 'pos' value. Thanks, -Stolee
On Tue, Mar 9, 2021 at 1:35 PM Derrick Stolee <stolee@gmail.com> wrote: > > On 2/25/2021 2:40 AM, Elijah Newren wrote: > > On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > >> > >> From: Derrick Stolee <dstolee@microsoft.com> > >> > >> The index_pos_by_traverse_info() currently throws a BUG() when a > >> directory entry exists exactly in the index. We need to consider that it > >> is possible to have a directory in a sparse index as long as that entry > >> is itself marked with the skip-worktree bit. > >> > >> The negation of the 'pos' variable must be conditioned to only when it > >> starts as negative. This is identical behavior as before when the index > >> is full. > > > > Same comment on the second paragraph as I made in the RFC series -- > > https://lore.kernel.org/git/CABPp-BGPJgA4guWHVm3AVS=hM0fTixUpRvJe5i9NnHT-3QJMfw@mail.gmail.com/. > > I apologize if I'm repeating stuff you chose to not change, but I > > didn't see a response and given the three typos left in previous > > patches, I'm unsure whether it was unaddressed on purpose or on > > accident. > > Yes, I dropped this one. How about this? > > The 'pos' variable is assigned a negative value if an exact match is not > found. Since a directory name can be an exact match, it is no longer an > error to have a nonnegative 'pos' value. I like it!
diff --git a/unpack-trees.c b/unpack-trees.c index 4dd99219073a..b324eec2a5d1 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -746,9 +746,12 @@ static int index_pos_by_traverse_info(struct name_entry *names, strbuf_make_traverse_path(&name, info, names->path, names->pathlen); strbuf_addch(&name, '/'); pos = index_name_pos(o->src_index, name.buf, name.len); - if (pos >= 0) - BUG("This is a directory and should not exist in index"); - pos = -pos - 1; + if (pos >= 0) { + if (!o->src_index->sparse_index || + !(o->src_index->cache[pos]->ce_flags & CE_SKIP_WORKTREE)) + BUG("This is a directory and should not exist in index"); + } else + pos = -pos - 1; if (pos >= o->src_index->cache_nr || !starts_with(o->src_index->cache[pos]->name, name.buf) || (pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf)))