shejialuo <shejialuo@xxxxxxxxx> writes: > On Mon, Jul 08, 2024 at 10:36:38AM -0400, Karthik Nayak wrote: >> shejialuo <shejialuo@xxxxxxxxx> writes: >> >> > The static function "report" provided by "fsck.c" aims at checking fsck >> > error type and calling the callback "error_func" to report the message. >> > However, "report" function is only related to object database which >> > cannot be reused for refs. In order to provide a unified interface which >> > can report either objects or refs, create a new function "vfsck_report" >> > by adding "checked_ref_name" parameter following the "report" prototype. >> > Instead of using "...", provide "va_list" to allow more flexibility. >> > >> > Like "report", the "vfsck_report" function will use "error_func" >> > registered in "fsck_options" to report customized messages. Change >> > "error_func" prototype to align with the new "vfsck_report". >> > >> > Then, change "report" function to use "vfsck_report" to report objects >> > related messages. Add a new function called "fsck_refs_report" to use >> > "vfsck_report" to report refs related messages. >> > >> >> Not sure I really understand why we need to do this. Why can't we simply >> add `const char *checked_ref_name` to the existing 'report' and >> propagate this also to 'error_func'. Why do we need all this parallel >> flows? >> > > Yes, we could just add a parameter "const char *checked_ref_name" to the > existing "report". This may seem the simplest way to do. However, it > will also introduce some trouble below: > > 1. "report" function should be exported to the outside, we need to > rename it to "fsck_report". Well, we need to change a lot of code here. > And we MUST do this, because "report" is a general name. When exporting > to the outside, it's not proper. > agreed. > 2. When we add a new parameter in "report", for all the "report" calls, > we need to pass this new parameter with NULL. > agreed too. > Use this way, we could do not change "report" function prototype and the > corresponding calls. Most importantly, we could let the caller feel > transparent. Using "report", caller can just ignore "checked_ref_name". > Also for "fsck_refs_report", we could ignore some UNUSED parameters. > > So I think this design is more elegant than just adding a new parameter > in the existing "report" function. > I understand what you're saying, I also checked and can see that there are 60 references to the `report()` function. So perhaps there is some merit in keeping it as is and adding a new 'report_refs()'. >> Apart from that, what does 'v' in 'vfsck_report' signify? >> > > Because I use "va_list" parameter, I want to emphasis on this. And this > provides flexibility that we could add a "fsck_report" function later. > There are many codes in git code base using this way. I just followed > this. > I see. Makes sense. >> Perhaps it is also because this commit is doing a lot of things and we >> could have simplified it into smaller commits? >> > > Actually, this commit is very clear. I just want to provide a unified > function "vfsck_report" here. And let the "report" use this function and > "fsck_refs_report" function use this. > > So I don't know whether we should split this commit into multiple > commits. They are just tied together.
Attachment:
signature.asc
Description: PGP signature
