Skip to content

refdb_fs: fix regression: failure when globbing for non-existant references#4665

Merged
pks-t merged 2 commits intolibgit2:masterfrom
neithernut:fix-refdb-glob
Jun 6, 2018
Merged

refdb_fs: fix regression: failure when globbing for non-existant references#4665
pks-t merged 2 commits intolibgit2:masterfrom
neithernut:fix-refdb-glob

Conversation

@neithernut
Copy link
Copy Markdown
Contributor

@neithernut neithernut commented Jun 1, 2018

Fixes #4664.

@neithernut
Copy link
Copy Markdown
Contributor Author

neithernut commented Jun 1, 2018

@jacquesg could you check whether this fixes the issue you reported?

@neithernut neithernut changed the title Fix refdb glob refdb_fs: fix regression: failure when globbing for non-existant references Jun 1, 2018
@jacquesg
Copy link
Copy Markdown
Contributor

jacquesg commented Jun 1, 2018

@neithernut Confirmed, I no longer see the previous failures. Thanks!

Comment thread src/refdb_fs.c

if ((error = git_iterator_for_filesystem(&fsit, path.ptr, &fsit_opts)) < 0) {
git_buf_free(&path);
return (iter->glob && error == GIT_ENOTFOUND)? 0 : error;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to check whether we have a globbing iterator or not? In the case where the iterator is not using globs, we're always using "refs/" as our entrypoint, and absence of that would certainly be unexpected. What is our current failure mode in that case? Do we error out or do we just report 0 references?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error gets propagated to the caller. I felt that it would be wrong to hide the error in this case. And the additional check hints at why GIT_ENOTFOUND can be ignored.

@pks-t
Copy link
Copy Markdown
Member

pks-t commented Jun 1, 2018

The fixup makes sense to me, thanks. Just waiting for some clarification on my comment and then I'd be good to merge

This commit fixes a regression introduced by

        20a2b02

The commit introduced an optimization for finding references using a
glob: rather than iterating over all references and matching each one
against the glob, we would iterate only over references within the
directory common to all possible references which may match against the
glob.

However, contrary to the `ref/` directory, which was the previous entry
point for the iteration, this directory may not exist. In this case, the
optimization causes an error (`ENOENT`) rather than the iterator simply
yielding no references.

This patch fixes the regression by checkign for this specific case.
@neithernut
Copy link
Copy Markdown
Contributor Author

Squashed in order to save a round-trip.

@pks-t
Copy link
Copy Markdown
Member

pks-t commented Jun 1, 2018

Waiting a few days whether there's feedback by others

@pks-t pks-t merged commit 20306d3 into libgit2:master Jun 6, 2018
@pks-t
Copy link
Copy Markdown
Member

pks-t commented Jun 6, 2018

Thanks again for your fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants