| Message ID | ZxZYZy-9deyT6I9a@ArchLinux (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/9] ref: initialize "fsck_ref_report" with zero | expand |
shejialuo <shejialuo@gmail.com> writes: > We passes "refs_check_dir" to the "files_fsck_refs_name" function which > allows it to create the checked ref name later. However, when we > introduce a new check function, we have to re-calculate the target name. > It's bad for us to do repeat calculation. Instead, we should calculate > it only once and pass the target name to the check functions. > > In order not to do repeat calculation, rename "refs_check_dir" to > "target_name". And in "files_fsck_refs_dir", create a new strbuf Nit: Why `target_name` and not simply `target`? > "target_name", thus whenever we handle a new target, calculate the > name and call the check functions one by one. > > Mentored-by: Patrick Steinhardt <ps@pks.im> > Mentored-by: Karthik Nayak <karthik.188@gmail.com> > Signed-off-by: shejialuo <shejialuo@gmail.com> > --- > refs/files-backend.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > [snip]
On Mon, Oct 21, 2024 at 09:34:31PM +0800, shejialuo wrote: > We passes "refs_check_dir" to the "files_fsck_refs_name" function which > allows it to create the checked ref name later. However, when we > introduce a new check function, we have to re-calculate the target name. > It's bad for us to do repeat calculation. Instead, we should calculate > it only once and pass the target name to the check functions. It would be nice to clarify what exactly is bad about it. Does it create extra memory churn? Or is this about not duplicating logic? > In order not to do repeat calculation, rename "refs_check_dir" to > "target_name". And in "files_fsck_refs_dir", create a new strbuf > "target_name", thus whenever we handle a new target, calculate the > name and call the check functions one by one. "target_name" is somewhat of a weird name. I'd expect that this is either the path to the reference, in which case I'd call this "path", or the name of the reference that is to be checked, in which case I'd call this "refname". > @@ -3539,6 +3538,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, > const char *refs_check_dir, > files_fsck_refs_fn *fsck_refs_fn) > { > + struct strbuf target_name = STRBUF_INIT; > struct strbuf sb = STRBUF_INIT; > struct dir_iterator *iter; > int iter_status; > @@ -3557,11 +3557,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, > continue; > } else if (S_ISREG(iter->st.st_mode) || > S_ISLNK(iter->st.st_mode)) { > + strbuf_reset(&target_name); > + strbuf_addf(&target_name, "%s/%s", refs_check_dir, > + iter->relative_path); > + > if (o->verbose) > - fprintf_ln(stderr, "Checking %s/%s", > - refs_check_dir, iter->relative_path); > + fprintf_ln(stderr, "Checking %s", target_name.buf); > + > for (size_t i = 0; fsck_refs_fn[i]; i++) { > - if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter)) > + if (fsck_refs_fn[i](ref_store, o, target_name.buf, iter)) > ret = -1; > } > } else { The change itself does make sense though. We indeed avoid reallocating the array for every single ref, which is a worthwhile change. I was wondering whether we could reuse `sb` here, but we do use it at the end of the function to potentially print an error message. Patrick
On Tue, Nov 05, 2024 at 08:11:46AM +0100, Patrick Steinhardt wrote: > On Mon, Oct 21, 2024 at 09:34:31PM +0800, shejialuo wrote: > > We passes "refs_check_dir" to the "files_fsck_refs_name" function which > > allows it to create the checked ref name later. However, when we > > introduce a new check function, we have to re-calculate the target name. > > It's bad for us to do repeat calculation. Instead, we should calculate > > it only once and pass the target name to the check functions. > > It would be nice to clarify what exactly is bad about it. Does it create > extra memory churn? Or is this about not duplicating logic? > Thanks, I will improve this in the next version. > > In order not to do repeat calculation, rename "refs_check_dir" to > > "target_name". And in "files_fsck_refs_dir", create a new strbuf > > "target_name", thus whenever we handle a new target, calculate the > > name and call the check functions one by one. > > "target_name" is somewhat of a weird name. I'd expect that this is > either the path to the reference, in which case I'd call this "path", or > the name of the reference that is to be checked, in which case I'd call > this "refname". > I felt quite hard to name this variable when I wrote the code. "refname" is not suitable due to we may check the reflog later by calling "files_fsck_refs_dir" function. So, we should use "path" here. Thanks, Jialuo
On Wed, Nov 06, 2024 at 08:32:19PM +0800, shejialuo wrote: > On Tue, Nov 05, 2024 at 08:11:46AM +0100, Patrick Steinhardt wrote: > > On Mon, Oct 21, 2024 at 09:34:31PM +0800, shejialuo wrote: > > > In order not to do repeat calculation, rename "refs_check_dir" to > > > "target_name". And in "files_fsck_refs_dir", create a new strbuf > > > "target_name", thus whenever we handle a new target, calculate the > > > name and call the check functions one by one. > > > > "target_name" is somewhat of a weird name. I'd expect that this is > > either the path to the reference, in which case I'd call this "path", or > > the name of the reference that is to be checked, in which case I'd call > > this "refname". > > > > I felt quite hard to name this variable when I wrote the code. "refname" > is not suitable due to we may check the reflog later by calling > "files_fsck_refs_dir" function. I anticipate that we'll likely have separate infra for checking reflogs as they are both stored in a different directory and because their format is completely different compared to normal refs. So there isn't really too much of a point to plan ahead for sharing logic here, I'd think, and thus "refname" might be a better fit. If that changes in the future we can still refactor the code. Patrick
diff --git a/refs/files-backend.c b/refs/files-backend.c index f246c92684..fbfcd1115c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3501,12 +3501,12 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store, */ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, struct fsck_options *o, - const char *refs_check_dir, + const char *target_name, struct dir_iterator *iter); static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, struct fsck_options *o, - const char *refs_check_dir, + const char *target_name, struct dir_iterator *iter) { struct strbuf sb = STRBUF_INIT; @@ -3519,11 +3519,10 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, if (iter->basename[0] != '.' && ends_with(iter->basename, ".lock")) goto cleanup; - strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path); - if (check_refname_format(sb.buf, 0)) { + if (check_refname_format(target_name, 0)) { struct fsck_ref_report report = { 0 }; - report.path = sb.buf; + report.path = target_name; ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_NAME, "invalid refname format"); @@ -3539,6 +3538,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, const char *refs_check_dir, files_fsck_refs_fn *fsck_refs_fn) { + struct strbuf target_name = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; struct dir_iterator *iter; int iter_status; @@ -3557,11 +3557,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, continue; } else if (S_ISREG(iter->st.st_mode) || S_ISLNK(iter->st.st_mode)) { + strbuf_reset(&target_name); + strbuf_addf(&target_name, "%s/%s", refs_check_dir, + iter->relative_path); + if (o->verbose) - fprintf_ln(stderr, "Checking %s/%s", - refs_check_dir, iter->relative_path); + fprintf_ln(stderr, "Checking %s", target_name.buf); + for (size_t i = 0; fsck_refs_fn[i]; i++) { - if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter)) + if (fsck_refs_fn[i](ref_store, o, target_name.buf, iter)) ret = -1; } } else { @@ -3578,6 +3582,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, out: strbuf_release(&sb); + strbuf_release(&target_name); return ret; }
We passes "refs_check_dir" to the "files_fsck_refs_name" function which allows it to create the checked ref name later. However, when we introduce a new check function, we have to re-calculate the target name. It's bad for us to do repeat calculation. Instead, we should calculate it only once and pass the target name to the check functions. In order not to do repeat calculation, rename "refs_check_dir" to "target_name". And in "files_fsck_refs_dir", create a new strbuf "target_name", thus whenever we handle a new target, calculate the name and call the check functions one by one. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> --- refs/files-backend.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)