Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jan 31, 2020

Fix: #3164

@ghost ghost requested review from efiop and shcheklein January 31, 2020 22:14
Copy link
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

please, check some comments

Suor
Suor previously requested changes Feb 3, 2020
Copy link
Contributor

@Suor Suor left a comment

Choose a reason for hiding this comment

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

Some dedup is needed. We also have similar message in dvc.lock.

@ghost
Copy link
Author

ghost commented Feb 4, 2020

I will handle the duplication in another pull request, as agreed on planning.
I think both methods can be abstracted.

@ghost
Copy link
Author

ghost commented Feb 5, 2020

@efiop , I already had the patch for it, since this PR isn't closed yet, I will upload it here.

@ghost ghost requested review from Suor and shcheklein February 5, 2020 00:14
@ghost ghost dismissed stale reviews from Suor and shcheklein February 5, 2020 00:14

reduce duplication

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@efiop efiop merged commit d9ed98b into treeverse:master Feb 5, 2020
@ghost ghost deleted the fix-3164 branch February 5, 2020 14:53
Comment on lines +65 to +70
blockers = [
info
for path, infos in lock[mode].items()
if path_info.overlaps(path)
if info not in (infos if type(infos) is list else [infos])
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is broken now. We create a list of info passed as param repeated 0 to several times, while in previous logic we were listing infos we've got from lock.

The right logic I guess is:

blockers = [
    blocker
    for path, infos in lock[mode].items()
    if path_info.overlaps(path)
    for blocker in (infos if isinstance(infos, list) else [infos])
    if blocker != info
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #3285.

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.

improve message for rwlock error

3 participants