Skip to content

removed volume identifier#1610

Open
loaflover wants to merge 4 commits intofox-it:mainfrom
loaflover:bugfix/remove-volume-identifier
Open

removed volume identifier#1610
loaflover wants to merge 4 commits intofox-it:mainfrom
loaflover:bugfix/remove-volume-identifier

Conversation

@loaflover
Copy link
Contributor

fixed depreciated function, since we have a better one implemented.

return cls(
serial=fs.ntfs.serial,
volume_uuid=get_volume_identifier(fs),
volume_uuid=fs.identifier,
Copy link
Member

Choose a reason for hiding this comment

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

I think the behavior silently changed of this. In get_volume_identifier, we preferred the volume GUID over the serial. However, in the new fs.identifier we prefer the serial over the volume GUID.

I'm not entirely sure which is nicer. Up for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im fine with either.

honestly i dont mind keeping the old function as deprecated function, it just seems kinda silly as AFAIK its barely used anywhere.

we could also easily change the order of fs.identifier, if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, the serial is already available in the line above this. Could be make this to be exclusively the volume UUID?

Copy link
Contributor

Choose a reason for hiding this comment

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

But wouldn't that put us back at square one? The serial wouldn't get yielded, so when an NTFS volume is using the old MBR approach, we wouldn't be able to differentiate between filesystems. Also, since the serial isn't used anywhere else, maybe we could just delete it altogether and rename this field?

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing loaflover:bugfix/remove-volume-identifier (b53c6c6) with main (053789f)

Open in CodSpeed

return cls(
serial=fs.ntfs.serial,
volume_uuid=get_volume_identifier(fs),
volume_uuid=fs.identifier,
Copy link
Member

Choose a reason for hiding this comment

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

On second thought, the serial is already available in the line above this. Could be make this to be exclusively the volume UUID?

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