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. 2. When we add a new parameter in "report", for all the "report" calls, we need to pass this new parameter with NULL. 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. > 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. > 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.
