Skip to content

Allow ubireader_list_files to recursively list files under a path#118

Merged
e3krisztian merged 1 commit intoonekey-sec:mainfrom
inomotech-foss:feat/list-recursive
Feb 19, 2026
Merged

Allow ubireader_list_files to recursively list files under a path#118
e3krisztian merged 1 commit intoonekey-sec:mainfrom
inomotech-foss:feat/list-recursive

Conversation

@scootermon
Copy link
Contributor

This is based on top of #117, but I'm happy to rebase it.

@qkaiser qkaiser self-assigned this Sep 1, 2025
@scootermon scootermon force-pushed the feat/list-recursive branch 3 times, most recently from b6ad0e6 to e2f6845 Compare September 15, 2025 17:42
@e3krisztian
Copy link
Contributor

There are 2 flags and features added in this MR:

  • print path instead of name
  • print recursively (requires path)

Both of them are passed down the functions, adding to their complexity.

At least the recursive flag should be handled as a separate path - this is most visible in print_dent, which conditionally become a recursive function.

I think this recursive listing should be implemented in a separate function, that handles the recursion: print_dent_recursive, which should be responsible to build the path for the dent and recursion, and this should call to print_dent to print a single entry with the path prefix (which is optional).

@scootermon scootermon force-pushed the feat/list-recursive branch 2 times, most recently from c096c9c to bed815d Compare February 13, 2026 19:26
@scootermon
Copy link
Contributor Author

scootermon commented Feb 13, 2026

@e3krisztian, I don't really see your vision on this. I went ahead and made some changes, but I fail to see how this is an improvement.

@e3krisztian
Copy link
Contributor

@scootermon I am very sorry for both being extremely slow and late with the review and answering and also to cause confusion with my comment.

This is how I was thinking about it (patch is based on your original change): 905445f (https://github.com/onekey-sec/ubi_reader/commits/list-recursive-suggestion/)

This is not tested, and does not include your recent changes in the history (rebase and improvement attempts).

Actually, you had a very similar attempt in c096c9c, but made the split on recursive at an earlier place, thus with duplicating some code, and also removed full-path printing support from print_dent (probably due to the comment confusion).


I think it is important for print_dent to keep its original scope to format and print only a single directory entry.
The formatting is also best kept in the now archaic one line format, which is more compact than the black/ruff one, because the code around it is written in that style.

@scootermon
Copy link
Contributor Author

scootermon commented Feb 17, 2026

Thanks for following up on this, @e3krisztian.

[...] and also removed full-path printing support from print_dent (probably due to the comment confusion).

So to be clear, this is a feature you want to keep? It was only added to print_dent because it's required for the recursive listing to make sense. Once print_dent and print_dent_recursive are separate print_dent will always be called with dent_path=None.

EDIT: Realized that the feature is still being used by the print_dent_recursive function so it makes sense to keep it.

I think it is important for print_dent to keep its original scope to format and print only a single directory entry.
The formatting is also best kept in the now archaic one line format, which is more compact than the black/ruff one, because the code around it is written in that style.

That's fair, I keep having to fight the urge to format the code.

I went ahead and "merged" your suggestion in 580dd88.

Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

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

@scootermon Thanks for the update!

I am happy with the code, and it can be merged - if you are also happy with it, could you squash the commits?

The default output of ubireader_list_files remains the same, but when you add '--recursive' it will recursively list all the inodes under the listpath.
It then also displays the absolute path to the files instead of just the name.

Co-authored-by: Krisztián Fekete <1246751+e3krisztian@users.noreply.github.com>
@e3krisztian e3krisztian merged commit 1e953b8 into onekey-sec:main Feb 19, 2026
1 check passed
@e3krisztian
Copy link
Contributor

@scootermon thank you for your contribution!

@scootermon scootermon deleted the feat/list-recursive branch February 19, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants