New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PFS-239] WalkFile and ListFile return incorrect results when directories and files have the same name #10050
base: master
Are you sure you want to change the base?
Conversation
src/server/pfs/server/compaction.go
Outdated
@@ -218,7 +216,9 @@ func (c *compactor) Validate(ctx context.Context, taskDoer task.Doer, id fileset | |||
var size int64 | |||
for i, result := range results { | |||
if errStr == "" && i != 0 { | |||
errStr = checkIndex(results[i-1].Last, result.First) | |||
if indxErr := fileset.CheckIndex(results[i-1].Last, result.First); indxErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, I generally think it is better to just use err
when working within a smaller scope (i.e. within the if statement), and only use specially named errors when necessary and in a larger scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also changed to err
in processValidateTask. Not sure if this is necessary. processShardTask also named error as err
commit4, err := env.PachClient.StartCommit(pfs.DefaultProjectName, repo, "master") | ||
require.NoError(t, env.PachClient.PutFile(commit4, "/a/a/a/a", &bytes.Buffer{})) | ||
require.NoError(t, err) | ||
//require.NoError(t, finishCommit(env.PachClient, repo, "", commit4.Id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I'll change this. The test was more of a draft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a draft and I was testing what happens when commit is finished and left open. Since this story is about open commits so I left this commented out here to indicate the commit is not finished..
No description provided.